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

Add Periodic BC #177

Open
wants to merge 14 commits into
base: jim-dev
Choose a base branch
from
Open

Add Periodic BC #177

wants to merge 14 commits into from

Conversation

thomasckng
Copy link
Collaborator

Haven't tested it yet. Just to put it here to see if anyone has any comments.

The method is suggested by @tsunhopang. I think the main downside is it requires an extra dimension. I think the geometry of the posterior should not be very complicated in most of the cases, so probably is still fine to have the extra dimension.

I couldn't think of any way to introduce the periodic BC except by adding features in the sampler level. I guess this is the best we can do in Jim for now.

@tsunhopang
Copy link
Collaborator

The current implementation of the prior class still does not consider the periodic BC. The prior class should do the following instead:

  1. Define gaussian prior on x, y
  2. Transform x and y into A and theta, where theta is periodic

Please see the following commits

@thomasckng
Copy link
Collaborator Author

thomasckng commented Jan 28, 2025

I think we want to define the prior on r and theta, as well as sample on x and y, so we need to define our prior on r and theta in the prior class and make a transform to put in sample_transform which turns r and theta to x and y. I think the implementation now only makes a prior on r and theta starting from a 2D-Gaussian, but it will still sample on r and theta. We just need to keep the PeriodicTransform then it should be fine.

@thomasckng
Copy link
Collaborator Author

Your implementation of the prior prevents negative r and solves a few bugs that I missed. Thanks!

@thomasckng
Copy link
Collaborator Author

On a side note, I think we don't need to define our priors from the base function now, as we now introduced the BoundToUnbound transform. The sampling space itself is confined, and there is no way for the sampler to walk outside of the boundaries, so we don't need to care about the sharp cuts at the boundaries of the priors anymore. I think it just makes the code less readable and uses a tiny bit more computational resources in each posterior evaluation.

@thomasckng
Copy link
Collaborator Author

test_periodic_uniform
This is a result of the test script in https://github.com/thomasckng/jim/blob/periodic-bc/test/integration/test_periodic_uniform.py. I am using one single local walker for this example. The y-axis of the plot shows where the walker is and the x-axis shows the number of steps. The figure shows that the walker can move from 2*pi to 0 in a step.

@tsunhopang
Copy link
Collaborator

Could you compare the resulting posterior with and without the periodic prior used? Given the likelihood peak at 0 (the same as 2pi), there could be some degree of visible improvement.

@thomasckng
Copy link
Collaborator Author

thomasckng commented Jan 28, 2025

Sure. Here is the result. The script is in https://github.com/thomasckng/jim/blob/periodic-bc/test/integration/test_periodic_uniform.py

For uniform prior without periodic boundary conditions,
histogram_uniform
walker_history_uniform

For the one with periodic BC,
histogram_periodic
walker_history_periodic

@thomasckng thomasckng marked this pull request as ready for review January 28, 2025 19:38
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