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
Nojump #4031
Nojump #4031
Changes from 13 commits
e7f6b6f
96f43c3
14b724e
8a0c60d
2eb56bb
d16bbda
e05ecfd
701cb10
5a4bd77
88f4395
081c127
dc06052
65c95c0
5acec8a
5b2f067
f0d57ce
146cc07
c8263f1
84527c8
635d15a
662a316
1ee4947
e5a10e6
112cb7d
43f3d0c
232be2a
f3e18a2
79927ca
08477ee
64a4b1d
f1b3588
0eccb0a
42c6112
f4904c6
a3b1a21
3c2a90b
22953e0
faf8db9
b8afc96
903cb29
535132e
e0e2b18
eec1758
0e64312
be901c3
5540da2
54b5651
a484bec
3ffdc27
ff881fa
39183f0
47b0e39
89d9eb7
997b221
70d23ba
e40ee0f
1351bc4
7ae4e29
49f5c01
95f08a4
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.
I can't quite remember how the description shows up. The best thing for you would be to try it out https://docs.mdanalysis.org/stable/documentation_pages/references.html#citations-using-duecredit and see if you're happy with how your paper is presented to users.
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.
How does this work when I go through the trajectory a second time:
I think there should be a test that it's still correct the second time around. Keeping state is tricky business and our readers' behavior is not always obvious (for instance, they should rewind when you start iteration).
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.
It makes ZERO sense to unwrap an already unwrapped trajectory, and so ideally the transformation should only ever act once... I think. I could adopt something similar to what positionaveraging does, and try and remember what direction the steps are going in. However, in principle, unwrapping an already unwrapped trajectory is basically a no-op, since the round term will always be zero. I suppose I'd just need to enact a reset.
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.
Note that the trajectory will not stay unwrapped. It’s not in memory, the coordinates are read from disk again. (Sorry if I misunderstood your comment… typing quickly before they really force me to switch to airplane mode)
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.
Only if the user is using the MemoryReader then changes are permanent but that’s handled automatically with transformations.
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.
Hmm, ok. Now it'll throw a warning that you are going in the opposite direction (or with an unequal stride). When it encounters this, it will reset the unwrapping procedure.
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.
We typically use UserWarning instead of bare Warning.
(We have a few dedicated warning classes in https://docs.mdanalysis.org/stable/documentation_pages/exceptions.html but we try to not proliferate more)
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.
remove
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.
We use
assert_allclose
nowadays but haven't updated everything.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.
mention VMD and fastpbc explicitly (include version numbers if you have!); we have other cases that we checked against a VMD implementation. We don't consider it shameful to give credit to other software ;-). More importantly, the more detail is provided for validation, the more confidence someone has in the correctness.
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.
Yeah, the TRICLINIC system just was too ugly. The synthetic system is easier to understand.
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.
What are the red values and how did you obtain them? Add a comment for provenance.
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.
My workflow for testing this was probably non-ideal. I basically made water orthogonal and non-orthogonal water boxes, checked that the results were equivalent to what I got from VMD/fastpbc. I then ran the algorithm on existing triclinic systems, and did some sanity checks. These are two atomic positions that happened to move the most. Comments are now included to highlight 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.
@jvermaas May I suggest making a test case with a single particle that travels at a constant speed through the box? That way it should be easy to verify the correcness of the unwrapping. You can do this directly in MDAnalysis using
You can then make this universe a test fixture.
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.
Oooh... That is cleverer @hmacdope . I'll let you know how I make out. I'll probably need to look at how to add PBC information to a universe, but there is probably documentation somewhere.
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 am going to make this comment a blocking review @jvermaas, let me know if you need any help, feel free to just ping me.
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 really like @hmacdope 's idea.
with some odd angle of the initial velocity and a smallish box and then wrap the straight shot trajectory with the
wrap()
transform.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.
RE setting PBC the following should work to give you a 10x10x10 orthorhombic box using the
dimensions
argument.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 scrapped the old benchmark and replaced it with a synthetic one. I think its still intelligible, and I can explain why the non-orthogonal number is what it is.
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 calling these variables "matrix", call them something like
ref_last_pos_166
or something that makes clearer what they are.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 you can replace the explicit loop with
By default, it has shape
(n_frames, n_atoms, 3)
so you have to change the lines below.