search

CobayaSampler / cobaya

Code for Bayesian Analysis
http://cobaya.readthedocs.io/en/latest/
Other
122 stars 125 forks source link

post: bugfix: chains should be detempered/reweighted at once (or bad weights) #322

Closed JesusTorrado closed 9 months ago

JesusTorrado commented 9 months ago

Should work.

@cmbant any feedback on the general approach before I go on with the TODO's?

TODO: do the same with Collection methods reweght, mean and cov

codecov-commenter commented 9 months ago

Codecov Report

Merging #322 (2a90ba9) into master (9502fce) will increase coverage by 0.00%. Report is 1 commits behind head on master. The diff coverage is 91.80%.

:exclamation: Current head 2a90ba9 differs from pull request most recent head 4081a63. Consider uploading reports for the commit 4081a63 to get more accurate results

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master     #322   +/-   ##
=======================================
  Coverage   84.78%   84.79%           
=======================================
  Files         124      124           
  Lines        9127     9159   +32     
=======================================
+ Hits         7738     7766   +28     
- Misses       1389     1393    +4
Files Coverage Δ
cobaya/input.py 83.13% <100.00%> (ø)
cobaya/likelihood.py 93.33% <ø> (ø)
cobaya/output.py 88.74% <100.00%> (+0.03%) :arrow_up:
cobaya/post.py 89.94% <100.00%> (+0.12%) :arrow_up:
cobaya/samplers/mcmc/mcmc.py 91.64% <ø> (ø)
cobaya/collection.py 89.48% <90.56%> (-0.26%) :arrow_down:
cmbant commented 9 months ago

Looks OK, I guess whatever you do it will be a bit messy

JesusTorrado commented 9 months ago

Looks OK, I guess whatever you do it will be a bit messy

Yeah, no clean solution. Finally added with_batch in the minimal number of places where spurious relative weights between chains can be introduced: when detempering and reweighting. I have added a test for this features in test_post_prior, on top of the equivalent getdist one already there.

I have also tried to solve #306

Waiting for your review/go-ahead.

JesusTorrado commented 9 months ago

OK to merge if tests pass?

Should we merge #319 and release 3.3.3? or better 3.4 since the there were a couple of major fixes?

cmbant commented 9 months ago

Sounds good to me, 3.4 could make sense.

cmbant commented 9 months ago

Which changes addressed #306?

JesusTorrado commented 9 months ago

Which changes addressed #306?

https://github.com/CobayaSampler/cobaya/pull/322/commits/3957c79d237d1293b908a5d93a58f3576c145ec8

Raises rtol from default 1e-5 to 1e-4. Could not verify, bc I have failed to reproduce the issue myself (reload with custom priors, at different temperatures).

cmbant commented 9 months ago

Thanks. I guess it depends on the order of magnitude of the various terms.