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

Fix sample-counting and resuming bugs #41

Merged
merged 8 commits into from
Nov 17, 2023
Merged

Conversation

kdolum
Copy link
Collaborator

@kdolum kdolum commented Jun 3, 2023

Fix bugs #10 and #20 as described in #10. In the usual case where Niter is a multiple of isave, all computed samples will be written out. The run can be resumed with some larger Niter and the samples will be correctly counted. If Niter is a multiple of thin but not of isave, the partial block of samples will be written at the end. In this case you cannot resume without deleting this partial block by hand. If Niter is not a multiple of thin, the extra samples will be computed but not written.of
Progress reports after resuming give the percentage of the new samples that have been computed, as well as the percentage of the total.

kdolum added 3 commits April 12, 2023 13:13
from the previous chain thin times.  Give an error unless the old
chain has a number of samples that is a multiple of isave/thin.
Write out the initial sample to the chain file and then
do Niter iterations of the MCMC process.  If
Niter = a*isave + b*thin + c, the number of samples in the output will
be a*isave/thin + b plus the initial sample.
Only allow resuming when the file contains only complete rows and
their count is one plus a multiple of isave/thin.
Raise an exception if isave is not a multiple of thin.
Progress reports include percentage of post-resume work accomplished.
Fix documentation of metaparameters following Paul Baker
@kdolum
Copy link
Collaborator Author

kdolum commented Jun 3, 2023

I tested many cases involving various values of Niter, but I don't have any test case that uses multiple threads. It would be good to test that before merging.

Better error message trying to resume with partial blocks.
@kdolum
Copy link
Collaborator Author

kdolum commented Aug 25, 2023

There was a bug involving parallel tempering in which partial blocks did not get written out except in the cold chain. Now that is fixed and I have tested it with multiple threads and everything seems to work.

@kdolum kdolum marked this pull request as ready for review August 25, 2023 19:22
@vhaasteren vhaasteren self-assigned this Oct 3, 2023
@vhaasteren
Copy link
Member

@kdolum, the works looks good. However, the integration tests are failing at the moment for reasons not related to your PR. I think it's good policy to fix those first before merging. Let me first have a look at that first.

You also write you haven't been able to test with MPI. Is that still correct? I saw your message regarding thermodynamic integration the other day, so perhaps you have been looking into this.

The tests do run locally on my workstation, so that's at least a good sign.

@kdolum
Copy link
Collaborator Author

kdolum commented Oct 3, 2023

I did test it with MPI and all seems well.

@vhaasteren
Copy link
Member

This PR depends on #44 to pass the integration tests. So that one first needs to be handled before we can merge this PR

Put a long format string in parentheses to make a line shorter and
satisfy flake8.
Copy link
Member

@vhaasteren vhaasteren left a comment

Choose a reason for hiding this comment

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

Looks good

# bare except ok
E722
# lambda expressions ok
E731
Copy link
Member

Choose a reason for hiding this comment

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

I didn't need to do these edits. But if they gave you problems locally, I don't see a reason to decline this

@vhaasteren vhaasteren merged commit 5b4a764 into nanograv:master Nov 17, 2023
10 checks passed
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