Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nlive calculation bug fix #119

Merged
merged 8 commits into from
Aug 19, 2020
Merged

nlive calculation bug fix #119

merged 8 commits into from
Aug 19, 2020

Conversation

williamjameshandley
Copy link
Collaborator

@williamjameshandley williamjameshandley commented Aug 18, 2020

Description

There is an off-by-one nlive bug that has been lurking for a while. This is most clearly seen in likelihoods with plateau regions, but has also been occuring in the computation of decreasing nlive values for the final set of live points.

In anesthetic, nlive is a column in the table, and therefore 'belongs' to the point corresponding to a row. It is used to compute the volume compression via the probability distribution:

P(t_i) = n_i t_i**(n_i -1)

n_i therefore corresponds to the number of live points for which row i is the lowest likelihood live point. This means for example that the final point at the end of a run should have nlive=1, not 0.

This PR adjusts the tests so that the values are correct, introduces a failing test for a 'wedding cake' likelihood, which is then fixed by subsequent commits

Checklist:

  • I have performed a self-review of my own code
  • My code is PEP8 compliant (flake8 anesthetic tests)
  • My code contains compliant docstrings (pydocstyle --convention=numpy anesthetic)
  • New and existing unit tests pass locally with my changes (python -m pytest)
  • I have added tests that prove my fix is effective or that my feature works

@williamjameshandley
Copy link
Collaborator Author

@lukashergt, this likely has implications for your work concerning re-weighting/slicing of nested sampling chains. It would likely be worth your pushing a PR with a failing test that captures that behaviour, which we can check if merging this provides a fix.

@codecov
Copy link

codecov bot commented Aug 18, 2020

Codecov Report

Merging #119 into master will increase coverage by 0.74%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   91.98%   92.72%   +0.74%     
==========================================
  Files          16       16              
  Lines        1460     1458       -2     
==========================================
+ Hits         1343     1352       +9     
+ Misses        117      106      -11     
Impacted Files Coverage Δ
anesthetic/gui/plot.py 97.05% <100.00%> (+2.88%) ⬆️
anesthetic/samples.py 100.00% <100.00%> (+1.63%) ⬆️
anesthetic/utils.py 97.32% <100.00%> (ø)
anesthetic/gui/widgets.py 98.86% <0.00%> (+4.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1202ce...a2e74f2. Read the comment docs.

@lukashergt
Copy link
Collaborator

@lukashergt, this likely has implications for your work concerning re-weighting/slicing of nested sampling chains. It would likely be worth your pushing a PR with a failing test that captures that behaviour, which we can check if merging this provides a fix.

I'm not sure I can turn this into a failing test.

What I have done is infer the evidence in two separate ways:

  1. by masking my NestedSamples data frame and recomputing live points:

    ns_masked._compute_nlive(ns_masked.logL_birth)

    which then let's me run ns_masked.ns_output() again to get a distribution for my logZ_new1

  2. by calculating prior and posterior volumes from the masked weights and doing

    logZ_new2 = logZ_old.mean() + np.log(V_post) - np.log(V_prior)

Should I expect that logZ_new1.mean() == logZ_new2 ?

@williamjameshandley
Copy link
Collaborator Author

Should I expect that logZ_new1.mean() == logZ_new2 ?

Or at least that they're close to within the error margins of ns_output.

Basically it would be good to do this for a standard example (e.g. on one of the ones in tests/example_data).

@andrewfowlie
Copy link
Collaborator

andrewfowlie commented Aug 19, 2020

I'm not that up to speed with the anesthetic codebase, but I have checked that the logZ method now gives an acceptable answer on a MultiNest run with plateaus.

Let me know if you want the MN run files or more details.

andrewfowlie
andrewfowlie previously approved these changes Aug 19, 2020
Copy link
Collaborator

@andrewfowlie andrewfowlie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loooks good, a couple of very small comments

bin/wedding_cake.py Outdated Show resolved Hide resolved
bin/wedding_cake.py Outdated Show resolved Hide resolved
@williamjameshandley
Copy link
Collaborator Author

@andrewfowlie if you're happy with the trimming down of the code, could you re-approve the PR so it can be merged?

@lukashergt let me know if you have any problem with/comments on this merge.

@williamjameshandley
Copy link
Collaborator Author

as the coverage checks were failing with the minor reorganisation I also added a couple of tests to bring us up to 100% coverage of samples.

andrewfowlie
andrewfowlie previously approved these changes Aug 19, 2020
Copy link
Collaborator

@lukashergt lukashergt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to have a version update for this PR (2.0.0b2) such that we can easier reference these changes later.

Other than that I'm not sure whether this indicates that there might be an over-correction, such that there still is an off-by-one error and whether that should be addressed here, or in #120.

@williamjameshandley
Copy link
Collaborator Author

I think it would be good to have a version update for this PR (2.0.0b2) such that we can easier reference these changes later.

Done, and I made 2.0.0-beta.1 an actual beta release with a tag.

Other than that I'm not sure whether this indicates that there might be an over-correction, such that there still is an off-by-one error and whether that should be addressed here, or in #120.

I plan to look quite carefully now at #120, as it's not impossible that off-by-one errors remain.

As @andrewfowlie is in NZ and likely asleep by now, could @lukashergt re-approve the version bump.

@williamjameshandley williamjameshandley merged commit f546bc7 into master Aug 19, 2020
@williamjameshandley williamjameshandley deleted the nlive_bug_fix branch August 19, 2020 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible issue in dlogX calculation
3 participants