Fix LALInference Gaussian Mixture Model prior for multiple values
Description
The Gaussian Mixture Model (GMM) prior in LALInferencePrior.c contains a bug that means that if there is more than one parameter being used, and values are drawn outside the prior ranges, the values get stuck and never change. This MR fixes this by making sure that previous values get removed if the value is outside the bounds. It also fixes another minor bug in the setting of a string variable.
The MR also makes a minor change to the ppe_likelihood code used when setting up a GMM from a prior file. It now check if the lower and upper bounds are finite independently of each other, rather than requiring both to be finite.
Notes: I have made the VARNAME_MAX
macro in LALInference.h bigger, changing it from 40 to 50. This is to accommodate more GMM variables. If this might be problematic for any other LALInference code I can remove this.
API Changes and Justification
Backwards Compatible Changes
-
This change introduces no API changes -
This change adds new API calls
Backwards Incompatible Changes
-
This change modifies an existing API -
This change removes an existing API
If any of the Backwards Incompatible check boxes are ticked please provide a justification why this change is necessary and why it needs to be done in a backwards incompatible way.
Review Status
I have verified that the code compiles and works for a pulsar parameter estimation example. As this touches both LALInference and lalapps/pulsar code cc @vivien, @john-veitch, @david-keitel, @karl-wette.
Merge request reports
Activity
- Resolved by John Douglas Veitch
Notes: I have made the VARNAME_MAX macro in LALInference.h bigger, changing it from 40 to 50. This is to accommodate more GMM variables. If this might be problematic for any other LALInference code I can remove this.
The size 40 is carefully chosen so that the LALInferenceVariableItem struct fits into a 64byte cache line on x86. If it's possible to squeeze your variables into 40 chars I'd rather keep it this way. It reduces RAM use and cache flushing.
assigned to @john-veitch
enabled an automatic merge when the pipeline for 6fcdf235 succeeds
@matthew-pitkin Approved and set for automatic merge
Thanks @john-veitch, thanks @karl-wette
added lalapps lalinference lalpulsar + 1 deleted label
added 1 commit
- c3b71128 - LALInferencePrior.c: minor bug fix in GMM prior draws
@john-veitch @karl-wette - I had to cancel the merge after finding that I hadn't completely fixed the problem. When I tried the code with three parameters in the GMM it was still failing. However, there was a very simple fix to this that I've now pushed.
The plot below shows draws (histogram) from the GMM prior (solid lines) when it has three parameters. Things look good.
mentioned in commit 60bb1b25