joshspeagle / dynesty

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

tackling duplication in storing the information about the runs #245

Closed segasai closed 3 years ago

segasai commented 3 years ago

Following the issue #244 and required changes, I've decided to finally get rid of error-prone way of storing the information about the saved/based/new runs. Now there is a common object that stores various parameters. This makes a lot of logic simpler, getting rid of hundreds of lines of code.

Since the test suite is now in better shape, I think most of the bugs were caught by the tests, so I'm pretty confident about the patch. The patch is quite big, but most of the changes are pretty straightforward.

(since when I edit the code it automatically goes through yapf, there are some white-space changes as well).

There are some stylistic improvements that can be further done, like reusing RunRecord from sampler in dynamicsampler. It's also probably worth putting additional checks on the sizes of various arrays. i.e. assert that lengths of various arrays in run records are the same, but it's good enough IMO

segasai commented 3 years ago

All the regression tests pass now, so it's ready for review

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 817711672


Changes Missing Coverage Covered Lines Changed/Added Lines %
py/dynesty/sampler.py 68 78 87.18%
py/dynesty/dynamicsampler.py 139 154 90.26%
py/dynesty/utils.py 12 30 40.0%
<!-- Total: 219 262 83.59% -->
Files with Coverage Reduction New Missed Lines %
py/dynesty/sampler.py 1 81.24%
py/dynesty/utils.py 3 41.47%
<!-- Total: 4 -->
Totals Coverage Status
Change from base Build 814234237: -1.1%
Covered Lines: 3293
Relevant Lines: 4681

💛 - Coveralls
joshspeagle commented 3 years ago

This is great! I've just finished going through and the internal refactoring is so much cleaner now, both for tracking things around and for possibly adding new quantities down the line. Happy to merge this in.

avivajpeyi commented 3 years ago

Hi @joshspeagle,

As this causes the way results are stored to change from the most up-to-date release on PyPI (v1.1), would it be possible to release a new version of dynesty on PyPI?

joshspeagle commented 3 years ago

There are currently a few more active issues being resolved, but the plan is to push a new version once those wrap up in the near-term future.