DEP: remove support for dynesty v2#1049
Conversation
ColmTalbot
added
sampling
ColmTalbot
force-pushed
the
remove-old-dynesty
branch
from
f228b63 to
469957e
Compare
GregoryAshton
left a comment
GregoryAshton
left a comment
There was a problem hiding this comment.
Overall looks good. Review tests would be needed to confirm sampling has not been impacted, but I don't see anything of significant concern.
| self.walks = (self.walks * delay + n_target / accept_prob) / (delay + 1) | ||
| self.kwargs["walks"] = min(int(np.ceil(self.walks)), _SamplingContainer.maxmcmc) | ||
| self.scale = tuning_info["accept"] | ||
| self.walks = (self.walks * delay + self.naccept / accept_prob) / (delay + 1) |
There was a problem hiding this comment.
Is this changing the way the walks is updated? Or am I misunderstanding the diff?
There was a problem hiding this comment.
I'm pretty sure the logic is the same, it's just some of the variable names that are different. The appearance of changes is mostly because I renamed the existing dynesty3_utils (compare with https://github.com/bilby-dev/bilby/blob/main/bilby/core/sampler/dynesty3_utils.py#L122-L126.)
| logl = args.loglikelihood(current_v) | ||
|
|
||
| blob = { | ||
| sampling_info = { |
There was a problem hiding this comment.
Is there a reason the scale is no longer tracked?
There was a problem hiding this comment.
This is included in the older version as a legacy thing from when we used to update the scale of proposals during the MCMC chain. I think we decided that that breaks things and so we don't use it anymore.
| self.thin = getattr(_SamplingContainer, "nact", 2) | ||
| self.maxmcmc = getattr(_SamplingContainer, "maxmcmc", 5000) * 50 | ||
| self.thin = kwargs.get("nact", 2) | ||
| self.maxmcmc = kwargs.get("maxmcmc", 5000) * 50 |
There was a problem hiding this comment.
Should we make this 50 factor configurable?
There was a problem hiding this comment.
We could, we would need to keep it matched to this line, https://github.com/bilby-dev/bilby/blob/main/bilby/core/sampler/dynesty3_utils.py#L377.
I'm going to suggest not in this PR to avoid introducing meaningful changes in the refactoring commit.
| thin = self.thin * iact | ||
| blob = {"accept": accept, "reject": reject, "act": act} | ||
| iact = ACTTrackingEnsembleWalk.integer_act(act) | ||
| thin = args.kwargs.get("thin", 2) * iact |
There was a problem hiding this comment.
Is this setting a default thin of 2? Is that new or just change of where the default is set?
There was a problem hiding this comment.
Just a difference in where it is defined between the two versions (https://github.com/bilby-dev/bilby/blob/main/bilby/core/sampler/dynesty_utils.py#L190).
3 participants
Drop support for older versions of dynesty for version 3.