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

Dead points and Truncation #354

Merged
merged 12 commits into from
Nov 14, 2023
Merged

Dead points and Truncation #354

merged 12 commits into from
Nov 14, 2023

Conversation

yallup
Copy link
Collaborator

@yallup yallup commented Nov 13, 2023

Description

Feature request for an additional convenience alias for dead_points matching the live_points style, and a truncate method that gives the consistent union of the two allowing recalculation of mid run statistics

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
  • I have appropriately incremented the semantic version number in both README.rst and anesthetic/_version.py

Copy link
Collaborator

@williamjameshandley williamjameshandley left a comment

Choose a reason for hiding this comment

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

This is a useful addition (I and @zixiao-h have certainly used this functionality)

Copying inline comments here:

@yallup: Currently tests are failing due to the second check here, is this defined correctly (should the logL_birth check be inclusive)?

@williamjameshandley: I think that the convention is consistent i.e. this version of the strictness ensures that the 'live points at iteration i' corresponds to 'live points at loglikelihood logL' in a sensible way.

I think the real issue is that samples.live_points() (i.e. the None functionality) is not returning the right thing. This should return 'the last set of live points', and at the moment returns the penultimate set.

I've pushed a new function 'contour' that aims to achieve this. Could you tidy up the docstrings with a new users eye?

Copy link

codecov bot commented Nov 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1a035b7) 100.00% compared to head (20155b3) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #354   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        34           
  Lines         2973      2984   +11     
=========================================
+ Hits          2973      2984   +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@williamjameshandley williamjameshandley mentioned this pull request Nov 14, 2023
6 tasks
@yallup
Copy link
Collaborator Author

yallup commented Nov 14, 2023

This is a useful addition (I and @zixiao-h have certainly used this functionality)

Copying inline comments here:

@yallup: Currently tests are failing due to the second check here, is this defined correctly (should the logL_birth check be inclusive)?

@williamjameshandley: I think that the convention is consistent i.e. this version of the strictness ensures that the 'live points at iteration i' corresponds to 'live points at loglikelihood logL' in a sensible way.
I think the real issue is that samples.live_points() (i.e. the None functionality) is not returning the right thing. This should return 'the last set of live points', and at the moment returns the penultimate set.
I've pushed a new function 'contour' that aims to achieve this. Could you tidy up the docstrings with a new users eye?

Ah that makes sense and is consistent with what I was seeing, I added a small change to cater for if a float (implicitly a logL val) is passed but it happens to form a valid index (i.e 10.0). Docstrings seem sensible to me now, added a bit there an an explict contour test

Comment on lines 1203 to 1206
except KeyError:
pass
return logL

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not covered by the tests -- I agree that your additional float check is safer than what was there before. Can we think of anything which the try except covers (given that it is now never hit by the tests?) if not, remove the extra code, and just move to straight return statements from the if elif else block.

Copy link
Collaborator

@williamjameshandley williamjameshandley left a comment

Choose a reason for hiding this comment

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

Please squash and merge!

@yallup yallup merged commit 7c30e72 into handley-lab:master Nov 14, 2023
22 checks passed
@yallup yallup deleted the aliases branch November 14, 2023 15:21
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.

2 participants