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.
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
Refactoring of MC moves #21
Refactoring of MC moves #21
Changes from 38 commits
b313867
a9d9f16
1ab9b75
15ba2e8
4af534a
70b3997
53f0616
0117630
5e52df2
469071d
a3ea44d
4ad677e
9c4d47b
b6d88cc
c5bce7a
df93003
acfe5e2
beb0bdc
0b57c2b
85a9ae4
67a9831
ea1695a
c992b04
0f049b2
21ff068
2074665
e339e44
231f830
9085a43
88ee9ba
030435c
3ec2aa0
0987cf2
fc9ede3
1daab8a
e419169
9ad8a23
9e000db
60e0814
e78992d
07fe368
6a2733b
6337ce5
12d3b10
47ab2cc
192604e
ba3817d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we standardize abbreviations or avoid them altogether, such as
n_moves
ornumber_of_moves
? Or maybe we can even sayn_attempts_per_step
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what if we run out of bytes with such long names?! I think that is a great point;
n_attempts_per_step
would probably be very clear.Edit: Since this will also apply to the langevin integrator move, we might not want to use the word "attempts" in the variable.
number_of_moves
might be more generally applicable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Langevin move is a special case of un-Metropolized move, where every step is accepted.
These moves are also distinct in that the Langevin move has "number of steps to take per move application" and a Monte Carlo barostat has a "number of volume adjustments to attempt per move application". In either case, something like
n_steps
andn_attempts
is probably more appropriate for those move types.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of
update
, we might want to refer to this asautotune
.frequency
would refer to number of events per step, butinterval
orperiod
refers to the number of steps between events.Suggest:
update_stepsize=True
->autotune=True
update_stepsize_frequency=100
->autotune_interval=100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be much clearer. I was struggling with coming up with a clear/general name for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also updated the name of the internal class function to be
self._autotune
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to make
self.autotune
be public, since it is reasonable to allow a user to change the autotune behavior on the fly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move
unit.nanometer
into the definition ofbox_vectors
. It's inconsistent to have some variables likepositions
contain units andbox_vectors
not contain units.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move
unit.nanometer
into the definition ofbox_vectors
. It's inconsistent to have some variables likepositions
contain units andbox_vectors
not contain units.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you open an issue and explain the error you are seeing on your machine? I just tested this on Linux, and it runs without error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I was going to do some more investigation separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a separate reporter for each
MCMove
, can we use the same reporter for allMCMove
classes and just have eachMCMove
object create its own HDF5 group to store its data? That drastically reduces the complexity of this example.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wiederm What are your thoughts? I feel that is definitely doable (probably some small changes to the API)...not sure if worth it though, since the idea was using these more for debugging (but that could make it even more relevant, simplify the interface to ensure we have good debug logging with minimal effort).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer importing the object just before you use it, since it makes it easier for people to copy-and-paste parts of examples this way. So instead, I suggest: