post: bugfix: chains should be detempered/reweighted at once (or bad weights) #322
Conversation
Codecov Report
❗ 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
|
|
Looks OK, I guess whatever you do it will be a bit messy |
Yeah, no clean solution. Finally added I have also tried to solve #306 Waiting for your review/go-ahead. |
JesusTorrado
changed the title
tests/test_post.py
Outdated
| @@ -105,8 +132,10 @@ def test_post_prior(tmpdir, temperature): | |||
| new_mean, new_cov = mpi.share() | |||
| # Noisier with higher temperature | |||
| atol, rtol = (1e-8, 1e-5) if temperature == 1 else (5e-4, 1e-3) | |||
There was a problem hiding this comment.
Is there a reason the results aren't basically identical regardless of temperature?
There was a problem hiding this comment.
Well spotted. This was there since a while ago, recent fixes make it unnecessary. Removing. (If anything, if detempering loaded chains we lose some precision due to the ".8g" txt formatting.)
| @@ -58,8 +58,7 @@ def current_logp(self) -> float: | |||
| :return: log likelihood from the current state as a scalar | |||
| """ | |||
| value = self.current_state["logp"] | |||
| # Unfortunately, numpy arrays are not derived from Sequence, so the test is ugly | |||
| if hasattr(value, "__len__"): | |||
| if isinstance(value, Sized): | |||
| value = value[0] | |||
There was a problem hiding this comment.
Sized does not imply indexable?
There was a problem hiding this comment.
Note the Sized form is also nearly ten times slower than hasattr, which I think may be why we were using that
There was a problem hiding this comment.
Sized simply implies that it has length (as opposed to Sequence, which is not compatible with numpy arrays). Reverting where applicable. I though you preferred stronger type checks and less duck-typing, but fine to me.
| @@ -202,13 +209,15 @@ class SampleCollection(BaseCollection): | |||
|
|
|||
| def __init__(self, model, output=None, cache_size=_default_cache_size, name=None, | |||
| extension=None, file_name=None, resuming=False, load=False, | |||
| temperature=None, onload_skip=0, onload_thin=1, sample_type=None): | |||
| temperature=None, onload_skip=0, onload_thin=1, sample_type=None, | |||
| is_batch=False): | |||
|
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? |
|
Sounds good to me, 3.4 could make sense. |
|
Which changes addressed #306? |
|
Thanks. I guess it depends on the order of magnitude of the various terms. |
Should work.
@cmbant any feedback on the general approach before I go on with the TODO's?
TODO: do the same with
Collectionmethodsreweght,meanandcov