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

MAINT: dynesty - reduce number of calls to add_live_points #872

Merged
merged 4 commits into from
Jan 10, 2025

Conversation

ColmTalbot
Copy link
Collaborator

A bug in dynesty (joshspeagle/dynesty#490) means that the chain can be corrupted if we write a checkpoint after catching an interrupt.

Currently, we call the unsafe method (add_live_points) more frequently than we need to. This PR makes it so that we only call this when making trace plots and after writing our main checkpoint file. This should reduce the number of calls drastically and practically eliminate our exposure to the issue.

Longer term, we may want to think about moving away from writing a new checkpoint file when exiting and rely on previously written checkpoints as pointed out in the linked issue, writing a checkpoint on exit will always be fundamentally unsafe, even if it can be made practically safe.

@GregoryAshton
Copy link
Collaborator

@ColmTalbot it looks like the CI is failing because dynamic doesn't have the add_live method - should this only apply to standard dynesty?

Copy link
Collaborator

@mj-will mj-will left a comment

Choose a reason for hiding this comment

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

Following the change to added_live, the changes look good to me.

@ColmTalbot have you had a chance to test this via bilby_pipe? I appreciate the tests should cover this but I think it would be good to be sure. What do you think @GregoryAshton ?

@ColmTalbot
Copy link
Collaborator Author

@ColmTalbot have you had a chance to test this via bilby_pipe?

I haven't, but it's a good idea. I'll try to set something up today.

@GregoryAshton
Copy link
Collaborator

All looks good to me too.

@ColmTalbot ColmTalbot force-pushed the avoid-dynesty-add-live branch from 0e6d732 to 4691081 Compare December 11, 2024 15:24
@mj-will mj-will added the sampling Issues about sampling algorithms, their efficiency and robustness label Dec 12, 2024
@mj-will mj-will added this to the 2.5.0 milestone Dec 12, 2024
@mj-will
Copy link
Collaborator

mj-will commented Jan 7, 2025

@ColmTalbot have you had a chance to test this via bilby_pipe?

I haven't, but it's a good idea. I'll try to set something up today.

Just following up on this @ColmTalbot, did you start a test(s) with this change?

@ColmTalbot
Copy link
Collaborator Author

@ColmTalbot have you had a chance to test this via bilby_pipe?

I haven't, but it's a good idea. I'll try to set something up today.

Just following up on this @ColmTalbot, did you start a test(s) with this change?

Yeah, I ran one test that previously triggered the issue that completed fine, it isn't an exhaustive test, but I can't think of a better one.

@mj-will
Copy link
Collaborator

mj-will commented Jan 7, 2025

@ColmTalbot have you had a chance to test this via bilby_pipe?

I haven't, but it's a good idea. I'll try to set something up today.

Just following up on this @ColmTalbot, did you start a test(s) with this change?

Yeah, I ran one test that previously triggered the issue that completed fine, it isn't an exhaustive test, but I can't think of a better one.

Sounds good, I think we can get this in then. If there are issues, hopefully the review will pick up on them.

@mj-will mj-will force-pushed the avoid-dynesty-add-live branch from 4691081 to 81f82ec Compare January 10, 2025 08:50
@mj-will mj-will merged commit 6fbb4bf into bilby-dev:main Jan 10, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sampling Issues about sampling algorithms, their efficiency and robustness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants