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.

Sign up for GitHub

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

Jump to bottom

Metropolis-Hastings Monte Carlo: Improve move_ratio kwarg #321

Closed
Alexsp32 opened this issue Dec 4, 2023 · 5 comments · Fixed by #325
Closed

Metropolis-Hastings Monte Carlo: Improve move_ratio kwarg #321

Alexsp32 opened this issue Dec 4, 2023 · 5 comments · Fixed by #325
Assignees
@Alexsp32
Labels
enhancement New feature or request
Milestone
End of 2023 update

Comments

@Alexsp32
Copy link
Member

Alexsp32 commented Dec 4, 2023

I would suggest two (breaking) changes to run_advancedmh_sampling (Code):

  1. Substitute move_ratio for its inverse: To me, it seems more logical for a move ratio of 0 to prevent the entire system from moving, while 1 causes the entire system to move. Doing it the other way round seems weird to me. Possibly we could split from move_ratio into stop_ratio and movement_ratio, where stop_ratio=1-movement_ratio and deprecate move_ratio to avoid breaking stuff.
  2. Ignore zero moves when deciding the fraction of the system to perturb: In systems where certain atoms should remain in place, they are assigned Dirac distributions centred at their starting position. Currently, their "movement" is included in the movement ratio during sampling. I would propose ignoring zero moves when applying the movement ratio to avoid generating moves containing identical structures to the previous ones.
@Alexsp32 Alexsp32 added the enhancement New feature or request label Dec 4, 2023
@Alexsp32 Alexsp32 added this to the End of 2023 update milestone Dec 4, 2023
@Alexsp32 Alexsp32 self-assigned this Dec 4, 2023
@Alexsp32
Copy link
Member Author

Alexsp32 commented Dec 4, 2023

I forgot to mention internal_ratio for the Ring polymer analogy, but same issue here.

@jamesgardner1421
Copy link
Member

Yeah I am in favor of attempting to improve the API. Whenever I used this I would always have to check which way round it was supposed to be. I wouldn't worry too much about breaking other people's code as I doubt we have many people counting on this behavior. Just make sure we effectively communicate the change.

You could consider adding an option so that only a single move is attempted at a time, that would be a good option for most cases.

Alexsp32 added a commit that referenced this issue Dec 5, 2023
@Alexsp32
Initial test for #321
922fb7a
Alexsp32 added a commit that referenced this issue Dec 5, 2023
@Alexsp32
Modify AdvancedMH.jl keyword handling #321
45e47ea 
- Update ThermalMonteCarlo.jl with new keywords movement_ratio and stop_ratio, where movement_ratio=1-stop_ratio
- Add warning for deprecated kwargs.
- Update documentation with information about new keywords
Alexsp32 added a commit that referenced this issue Dec 5, 2023
@Alexsp32
Drop compat for AdvancedMH.jl<0.8 for #320,#321
3e18b56
@Alexsp32
Copy link
Member Author

Alexsp32 commented Dec 5, 2023

I've done some limited testing on 45e47ea. Both the test suite and the system I'm modelling behave properly and sampling stays stuck on the same structures less for my larger system.

@Alexsp32 Alexsp32 mentioned this issue Dec 5, 2023
Closed
Alexsp32 added a commit that referenced this issue Dec 11, 2023
@Alexsp32
Initial test for #321
f22e003
Alexsp32 added a commit that referenced this issue Dec 11, 2023
@Alexsp32
Modify AdvancedMH.jl keyword handling #321
f16d792 
- Update ThermalMonteCarlo.jl with new keywords movement_ratio and stop_ratio, where movement_ratio=1-stop_ratio
- Add warning for deprecated kwargs.
- Update documentation with information about new keywords
Alexsp32 added a commit that referenced this issue Dec 11, 2023
@Alexsp32
Drop compat for AdvancedMH.jl<0.8 for #320,#321
043da11
@Alexsp32 Alexsp32 mentioned this issue Dec 11, 2023
Merged
@Alexsp32 Alexsp32 linked a pull request Dec 11, 2023 that will close this issue
Update AdvancedMH.jl wrapper for Monte Carlo sampling. #325
Merged
@reinimaurer1
Copy link
Member

In general, the keywords should be as descriptive as possible. move_ratio to me also suggests that 1 is a high "ratio".
Please don't forget to describe the behaviour in the function and to update the docs/tutorial

@Alexsp32
Copy link
Member Author

Alexsp32 commented Jan 8, 2024

Docs are updated to include information about the changes: https://nqcd.github.io/NQCDynamics.jl/previews/PR325/initialconditions/metropolishastings/

Alexsp32 added a commit that referenced this issue Jan 25, 2024
@Alexsp32
Update AdvancedMH.jl wrapper for Monte Carlo sampling. (#325)
b6c2a73 
1. AdvancedMH or one of its dependencies may have updated a keyword from "initial_config" to "initial_params".
This caused NQCD to not deliver initial positions to the sampling chain correctly, leading to weird results for systems not centred around 0. 

2. Updated new keywords `movement_ratio` and `stop_ratio`, where `movement_ratio=1-stop_ratio` as the old `move_ratio` was confusing. Warning for deprecated kwargs was added and documentation updated with information about new keywords

* Drop compat for AdvancedMH.jl<0.8 for #320,#321

* Version bump for #325
@Alexsp32 Alexsp32 mentioned this issue Apr 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Alexsp32 Alexsp32

Labels
enhancement New feature or request
Projects
None yet
Milestone
End of 2023 update
Development

Successfully merging a pull request may close this issue.

Update AdvancedMH.jl wrapper for Monte Carlo sampling. NQCD/NQCDynamics.jl
3 participants
@reinimaurer1 @jamesgardner1421 @Alexsp32