California-Planet-Search / radvel

General Toolkit for Modeling Radial Velocity Data
http://radvel.readthedocs.io
MIT License
57 stars 52 forks source link

Implement Nested Sampling (and Bayesian Evidence Calculations) #383

Open vandalt opened 11 months ago

vandalt commented 11 months ago

Hi!

I recently tried to play with nested sampling in Radvel and so far it's working pretty well. If this gets merged eventually, it would fix #149.

Things already done

The PR includes the following:

Things left to do

Some things I was wondering and/or that are still left to do:

Question(s) about priors

One thing I found tricky is how to handle certain priors that don't directly depend on one parameter or that complement other priors (e.g. ecc < 1, K > 0). Some avenues I thought about to handle those are:

I'm open to any comments/suggestions about all of the above of course.

Thank you!

vandalt commented 10 months ago

Marking as ready for review, as I have WIP tutorials and I think the questions in the PR will determine the remaining development work.

bjfultn commented 10 months ago

This appears to be failing CI but it makes it a little unclear because its a cross-fork PR. Do the tests pass for you if you manually run them as defined in the travis.yaml file?

vandalt commented 10 months ago

Hi @bjfultn,

Most were passing when I ran it manually. However I will re-run them and add new ones once the PR is complete.

To finish the PR, I still need to address the points with check boxes above. I made the PR ready for review to get feedback/opinions on these implementation details, but we can turn it back to draft to avoid confusion. If there are no preferences regarding the checklist above, I'll tackle those tasks to the best of my knowledge and we can then discuss it in a final review.

bjfultn commented 10 months ago

I think you should choose one nested sampler implementation and run with it. I don't think its necessary to support multiple implementations.

I think it would be good to have some nested sampler documentation and a tutorial before deploying this to production. In that documentation there should be some explanation about the advantages and disadvantages of using nested sampling over differential evolution and the situations where its appropriate.

I like your ideas for priors but I have two comments, 1) We should not remove or drastically change the way the existing priors. Backwards compatibility is important. You should add new priors where necessary, even if they are sort of duplicates of ones we have already. and 2) Make sure that it is made clear in the tutorial the importance of appropriate priors when using nested sampling. I think that it would be good to run the check_proper_priors() method just before nested sampling starts and have it error out with an explanation to the user that they need to make adjustments to their priors. I don't think we should adjust our existing priors to work with nested sampling if they don't already.