-
Notifications
You must be signed in to change notification settings - Fork 758
Added NoJump on-the-fly example to MSD documentation (Issue #4169) #5165
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
base: develop
Are you sure you want to change the base?
Conversation
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.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.
IAlibay
left a comment
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.
Not reviewing the code but enforcing license - please don't change the license.
|
Thank you for pointing this out! I’ve updated the file to restore the correct license header ( |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5165 +/- ##
========================================
Coverage 92.72% 92.72%
========================================
Files 180 180
Lines 22472 22472
Branches 3188 3188
========================================
Hits 20837 20837
Misses 1177 1177
Partials 458 458 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
orbeckst
left a comment
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.
Please demonstrate to yourself and your reviewers that your doc update is correct.
package/AUTHORS
Outdated
| - Raúl Lois-Cuns | ||
| - Pranay Pelapkar | ||
| - Shreejan Dolai | ||
| - **Tanisha Dubey (https://github.com/tanii1125)** |
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.
follow existing formatting, please
package/MDAnalysis/analysis/msd.py
Outdated
| .. code-block:: python | ||
| import MDAnalysis as mda | ||
| from MDAnalysis.transformations import NoJump | ||
| u = mda.Universe(TOP, TRAJ) | ||
| # Apply NoJump transformation to unwrap coordinates | ||
| nojump = NoJump(u) | ||
| u.trajectory.add_transformations(nojump) | ||
| # Now the trajectory is unwrapped and MSD can be computed normally: | ||
| from MDAnalysis.analysis.msd import EinsteinMSD | ||
| MSD = EinsteinMSD(u, select="all", msd_type="xyz") | ||
| MSD.run() |
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.
Please run code and post images of the output graph. Demonstrate to the reviewers that the example does what you say it is doing.
When you run code, show us the code that you're running, including the input files.
89d9658 to
b81e783
Compare
orbeckst
left a comment
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.
Is RANDOM_WALK a good example, namely, did you check that it was actually generated with periodic boundary conditions?
In order to demonstrate that the example is working, I'd expect to see a comparison between omitting and using NoJump.
example/msd_nojump_demo.py
Outdated
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.
Thanks for showing the code. However, we do not add example scripts to this repository. Please remove it. Just copy and paste the code in the PR comments for review.
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.
Thanks for clarifying. I’ll remove the example script from the repository and instead include the code directly in the PR comments for review.
package/MDAnalysis/analysis/msd.py
Outdated
| In MDAnalysis you can use the | ||
| :class:`~MDAnalysis.transformations.nojump.NoJump` | ||
| transformation. | ||
| :class:`~MDAnalysis.transformations.nojump.NoJump` |
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 indentation looks off. Please correct.
Thanks for the clarification, After checking more carefully, I realized that:-
Instead of assuming wrapping is present in the input data, I will construct a deterministic test where a periodic box is defined and an artificial jump across the boundary is introduced between consecutive frames. This allows us to verify the intended behavior directly. |
|
@orbeckst the code i used to demonstrate this -- |


This PR updates the MSD documentation inside
MDAnalysis.analysis.msdto show users how to correctly apply the NoJump transformation using on-the-fly trajectory transformations.Issue #4169 noted that although NoJump exists, there was no example showing how to use it directly in MDAnalysis.
This PR adds:
Fixes: #4169
I would appreciate any feedback or suggestions for improvement.
Reviews are very welcome, and I’m happy to revise the PR based on your guidance.
📚 Documentation preview 📚: https://mdanalysis--5165.org.readthedocs.build/en/5165/