Updated LAMMPS dump file topology parser #4995
Conversation
![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 #4995 +/- ##
==========================================
Coverage 93.42% 93.42%
==========================================
Files 177 189 +12
Lines 21865 22960 +1095
Branches 3079 3095 +16
==========================================
+ Hits 20427 21450 +1023
- Misses 986 1059 +73
+ Partials 452 451 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I am aware I need to add tests for the new information, but I am slightly unsure how many new dump files is suitable to add to the data folder. I have tested the new additions with files on my own machine but don't want to add more files than necessary |
|
If they are small then adding a few is not really an issue. How many do you want to add? Are you able to make very minimal examples? |
|
@jaclark5 would you be able to have a look, given your expertise with LAMMPS? |
|
@jpkrowe I'd start with adding ~3 small files. If reviewers find that this too much they'll tell you. It's also possible to create very small files inline (using StringIO) – so if you can make your examples ~20 lines or less then that's also a neat way to keep tests contained. Writing good tests is an essential skill and we highly appreciate it when someone puts some thought and effort into it! |
|
Ok thanks I will make a handful of small examples and add tests for them |
|
@jpkrowe looks like a great contribution! Tag me when you've added the tests and I can give it a thorough review. |
|
@jaclark5 I have now added tests for the additions. Thanks for having a look! |
|
Hey @jpkrowe thanks for the quick turn around. I'll try to get to this in the next few days. In the meantime, it looks like there are a few conflicts to be resolved. |
There was a problem hiding this comment.
Looks like a great addition that will improve MDAnalysis handling of LAMMPS dump files. Items I would like to see addressed are:
- There is one instance of a nested loop that occurs over every atom where unneeded iterations can be avoided.
- Since elements are being checked and supported, I think the masses can be "guessed" at this stage. It looks like
guesser.tableshas that information.
Other than that I made some suggestions to reduce redundant code.
|
@jaclark5 Thanks for the detailed response. I have added the suggested changes and added tests for them |
There was a problem hiding this comment.
Looks great! There's one check that timed out after 50 min. I profiled your tests locally and they are all running in less than 0.005 seconds for me. @orbeckst what is the protocol for this situation? I ran the CI again with the same result.
|
Try restarting it again. However, I’ve seen these macOS runners timing out on a few PRs. @IAlibay @p-j-smith @yuxuanzhuang as some of the CI team, have you got any insights? Should we merge PRs even if the macOS-13 runners are not passing due to timeouts? |
|
I restarted that runner, we will see what happens. |
Fixes #3449
Changes made in this Pull Request:
-This PR allows for greater flexibility when using LAMMPS dump files for topology information, and allows mass, charge and element information to be read from the file if provided.
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--4995.org.readthedocs.build/en/4995/