Added dt support for xtc and trr trajectories.#4908
Added dt support for xtc and trr trajectories.#4908orbeckst merged 27 commits intoMDAnalysis:developfrom
xtc and trr trajectories.#4908Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4908 +/- ##
========================================
Coverage 93.61% 93.61%
========================================
Files 177 177
Lines 21894 21907 +13
Branches 3095 3100 +5
========================================
+ Hits 20495 20508 +13
Misses 946 946
Partials 453 453 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Overall this is looking pretty solid. Please see comments below. Could you please address them?
orbeckst
changed the title
xtc and trr trajectories.xtc and trr trajectories.
|
@pbuslaev please resolve conflicts. Ping when you've addressed comments and then I'll have a look again. Please don't resolve comments — I'll do that when I review again. Thanks. |
|
@orbeckst I think all should be addressed now. |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Your changes overall look good and I only have minor comments.
However, all the tests fail. Please check where your code broke them.
|
Hi @orbeckst As I mentioned before
So here I am waiting for your input |
- removed XTC test in the DCD tests for setting dt in the writer - ensure that dt is actually only changed when writing
|
I changed the test. These tests are old code and it makes little sense that we are testing XTC together with DCD. Furthermore, the test did not even test the XTCWriter properly: we had set dt on reading, presumably because that was the only way to make it pass for the XTCWriter, but then setting dt on writing did not do anything else. Only with your addition is the XTCWriter now able to use dt, and you test this functionality elsewhere. |
Do not explicitly set dt for the Writer. (In the past, this did not do anything for TRR but with this new change it forces the recalculation of times and that introduces inaccuracies at rtol=1e-6. The test does not really check dt so no need to set it.
|
Let's see if these changes to the tests make the tests more robust. I note that we seem to have been assuming in the past that setting dt for XTC and TRR worked, even though it did not. |
|
@pbuslaev I think the tests are now passing. Could you please address the small doc issues and then it looks ready for merging. |
|
(I enabled auto-merge, i.e., the PR will get merged as soon as tests pass and the PR is approved.) |
Head branch was pushed to by a user without write access
|
@orbeckst I think I have addressed all the comments. The tests are passing and docs are rendered correctly. |
|
Thank you for your help and review @orbeckst |
|
Hooray, your first PR is merged 🎉 ! Thank you for all your work. I hope we see more contributions from you in the future. |
3 participants
Fixes #4905
Changes made in this Pull Request:
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--4908.org.readthedocs.build/en/4908/