always save the latest lognl in the demargin hdf #4900
Conversation
There was a problem hiding this comment.
@WuShichao See comments. I thinn tk probably the right place for changes here is actually on the model side.
@cdcapano Thoughts?
| def callmodel(arg): | ||
| iteration, paramvals = arg | ||
| # calculate the logposterior to get all stats populated | ||
| model.update(**{p: paramvals[p] for p in model.variable_params}) | ||
| _ = model.logposterior | ||
| stats = model.get_current_stats() | ||
| if hasattr(model, 'submodels'): |
There was a problem hiding this comment.
@WuShichao Wouldn't it make more sense for heirarchical models to make it so that get_current_stats includes any submodels rather than hardcoding knowledge about submodels into this script? It's best to keep information compartmentalized and use clearer interfaces to avoid technical debt in the future and prevent flexibility.
Similarly, why do we need to hardnode anything about lognl here? Maybe it should just be included in the stats when it is a fixed number like this, no?
There was a problem hiding this comment.
The lognl issue is not the hierarchical-model-only problem; whenever users use this script and lognl is much larger than loglr, the same problem will occur. So instead of modifying all likelihood models, I only change in one place. The reason why I hardcode lognl is I want to match the keys in the original PE file.
There was a problem hiding this comment.
@WuShichao (1) You are explicitly checking through submodels here. That really should be handled by the hierarchical model itself.
(2) lognl should probably just be a model stat if it is a constant. No modifications are then needed here.
Standard information about the request
This is a: bug fix,
This change affects: inference
This change changes: result presentation / plotting, scientific output
This change will: nothing
Motivation
There is a precision issue in lognl calculation, during reconstruct process, model will get slightly different lognl and loglr values (so loglikelihood). This becomes a problem when loglr is very small compared to lognl, which numerical precision is domainted by lognl. If still use the original lognl before the reconstruct process,
pycbc_inference_plot_posteriorwill get wrong loglr/snr for plotting.Contents
Save the latest
lognlinto the demargin hdf file.