search

joshspeagle / dynesty

Dynamic Nested Sampling package for computing Bayesian posteriors and evidences
https://dynesty.readthedocs.io/
MIT License
347 stars 76 forks source link

remove explicit periodic boundary application #153

Closed ColmTalbot closed 5 years ago

ColmTalbot commented 5 years ago

Hi @joshspeagle, @GregoryAshton and I have been discussing the periodic/reflective boundary situation, I think it has gotten a little confused.

This PR should revert the boundaries to the way it was before https://github.com/joshspeagle/dynesty/pull/129, i.e., users pass a list of "periodic" parameters and the map is then applied by the user. I'm don't think it's possible to achieve this through reversions as part of the changed code was introduced in https://github.com/joshspeagle/dynesty/commit/eb3b313b9881f0ececcfee2bc85887b0fd9d95b0.

Another alternative is to provide a ternary option of none/periodic/reflective, but that might be more work to code up and confuse the logic a little more.

We're happy to leave the final decision on implementation up to you, but as far as I can tell the current version (after https://github.com/joshspeagle/dynesty/pull/151) does not allow for reflective boundaries.

ColmTalbot commented 5 years ago

@GregoryAshton just pointed out that this will leave us with the issue described in https://github.com/joshspeagle/dynesty/issues/128. I think he's looking at coding up the ternary case.

joshspeagle commented 5 years ago

Okay. So should I expect to merge in this and #154? Or just wait until #154 is ready and then close this PR?

ColmTalbot commented 5 years ago

I would use #154 over this.

joshspeagle commented 5 years ago

Okay. Then I'll wait until @GregoryAshton is finished up with that before merging it in and closing this. Thanks.