joshspeagle / dynesty

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

Add reflective or periodic bounds #154

Closed GregoryAshton closed 5 years ago

GregoryAshton commented 5 years ago

@joshspeagle and @ColmTalbot , I've removed the WIP as I've done the following check: run a single job with one periodic, one reflective, and one "None" boundary and checked that the flags get propagated down correctly. There are a lot of changes though, so I'd appreciate some eyes to make sure I haven't broken anything.

@joshspeagle are you happy with the name "nonbounded"?

joshspeagle commented 5 years ago

nonbounded seems fine to me!

GregoryAshton commented 5 years ago

Okay, I've run this on a test case. In these, mass_ratio has a reflective boundary, ra has a periodic boundary and chirp_mass as no boundary. I intentionally set the simulated values of ra and mass_ratio close to the boundaries to see what happens.

This is the behaviour:

image

If on the other hand, I make the mass_ratio have no boundary then we get

image

This confirms to me that the "reflective" condition is user-configurable and acting as we want it to. Whether or not someone wants it to be reflective/periodic is up to them :)

So I'm personally happy for this to be merged, but of course @joshspeagle you should make any stylistic changes you require.

Could I also ask if you have a timescale for the next release in mind?

GregoryAshton commented 5 years ago

I added a check so that if the parameter is both periodic and reflective a ValueError is raised.

joshspeagle commented 5 years ago

Great — I would’ve added in the last check, so thanks for that. The behavior looks stable, so I’m happy to merge soon.

As for the next release, I’m currently bogged down in job applications but pushing hard to find enough time to get all the PRs and issues resolved by next week for the next release. Please feel free to bug me if you’d like things sooner; I don’t mind at all :).

GregoryAshton commented 5 years ago

Great to hear. A release would really help us out (we have a bunch of people lined up to run things and it's much easier to tell them pip update !). If you want to assign me any issues etc feel free.

Good luck with the job apps