adaptive translation / rotation scale factor #140
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Needs to properly handle moves / rotates when there are no adsorbates in the system. Right now, these count as proposed moves (+1 in markov_counts) but always "fail" since there is nothing to move / rotate. These shouldn't be included in the acceptance rate calculations. |
|
I am now certain that the scaled rotation_matrix function incorrect! :) |
…o both directions; variables renamed to match Arvo paper explicitly to make it easier to compare.
|
I think the main problem with the rotation matrix is fixed now with the last commit, mainly that the rotation about the z-axis wasn't centered so it went in only one direction. My code for visualizing the results of the rotation_matrix is here: https://github.com/paulboone/mcH2O/blob/master/RotationMatrixTest.jl |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adjusts the MC translation move maximum axis perturbation within the bounds of 0.01 - 2Å and Adjusts the MC rotation move maximum rotation within the bounds of 0.0001 to 1.0 of a complete rotation (i.e. 1.0 = 2pi radians).
This PR is mostly to get a review prior to making final changes!
Please take a CLOSE look at the rotation_matrix function. I am not sure at all if the math is correct here. I do know that doing it this way accomplishes the desired effect of the rotation scaling parameter being adjusted correctly when using a system of dense water molecules, which implies it is correct, but I definitely did not derive it. Without the diagm multiplication, the scaling parameters do not affect the acceptance %s since I think the molecule is rotated by a small scaled value but then flipped across the z axis (I think). This will almost never be a good idea with water molecules, so the rotations always failed without that diagm term.
Currently I've done the adjustment based on the acceptance % since the last adjustment, but I'm beginning to think it should be based on the all-time acceptance %, instead. Thoughts?
I have written no additional tests for this (yet). It looks like the only existing tests for the rotation_matrix function are molecule_test.jl:159:192, which still pass. It looks like the the generic_rotation_test.jl file is stale?
Let me know if you have naming / comment / style conventions or any other comments / thoughts / requests at all.