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 PTSwap Output #48

Merged
merged 8 commits into from
Dec 7, 2023
Merged

Conversation

AaronDJohnson
Copy link
Collaborator

@AaronDJohnson AaronDJohnson commented Dec 5, 2023

  • Changes the PTSwap method to also scatter a numpy array of acceptances allowing the sampler to output PT swap acceptance rates as it did previously.

@kdolum
Copy link
Collaborator

kdolum commented Dec 5, 2023

The code looks right to me. Is the functionality documented somewhere that with each chain it outputs the acceptance rate of swaps between this chain and the next hotter one?
The header comment of PTswap discusses a return value swapReturn. Does that not exist? Maybe we should remove it from the documentation if so.
Would it be better to remove self.nswap_accepted = 0 from the initialization code?

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.

Aside from what @kdolum says, you also seem to have committed new versions of two example notebooks. But the notebooks do not seem to have changed, except for the figures. Looking at the figures nothing is updated.

So please remove the ipynb edits from the PR, they do not seem different. Or create a new PR that doesn't contain them

examples/gaussian_likelihood.ipynb Outdated Show resolved Hide resolved
examples/simple.ipynb Outdated Show resolved Hide resolved
@AaronDJohnson
Copy link
Collaborator Author

AaronDJohnson commented Dec 6, 2023

@vhaasteren I will remove the ipynbs and make a separate PR. These are extremely minor errors that my linter picked up.

@AaronDJohnson
Copy link
Collaborator Author

@kdolum I've updated the docstring to hopefully make it clear that the function does not return swapAccept anymore, but it does still use the nswap_accepted parameter. It's necessary to keep this in the initialization so that we can increment it when swaps happen. The actual acceptance rates are computed elsewhere in the code.

@AaronDJohnson
Copy link
Collaborator Author

AaronDJohnson commented Dec 6, 2023

Notebook changes have been removed and moved to #49

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 to me now. Also happy, @kdolum?

@kdolum
Copy link
Collaborator

kdolum commented Dec 6, 2023

Looks good to me now too.

@vhaasteren vhaasteren merged commit af30a15 into nanograv:master Dec 7, 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.

3 participants