Add ddcMD file format supports #4252
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 the developer mailing list 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.
Linter Bot Results:Hi @XiaohuaZhangLLNL! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #4252 +/- ##
===========================================
- Coverage 93.40% 92.08% -1.33%
===========================================
Files 169 186 +17
Lines 22204 23710 +1506
Branches 4064 4153 +89
===========================================
+ Hits 20740 21833 +1093
- Misses 948 1351 +403
- Partials 516 526 +10
☔ View full report in Codecov by Sentry. |
|
@XiaohuaZhangLLNL I am excited for new formats to be added! Do you have a specification for the formats you could share on an issue so we can possibly discuss what is needed there? Just so we can fully understand the format (especially if it's binary). |
We hope MDAnalysis could support file formats of our ddcMD MD engine (https://github.com/LLNL/ddcMD). ddcMD has both text and binary file formats. The text file format can be find in: https://github.com/LLNL/ddcMD/blob/master/examples/waterbox/snapshot.mem/atoms%23000000 The binary file is similar to text file format but just in binary format. I can provide an example if it is needed. |
|
@XiaohuaZhangLLNL is there any chance that the ddcMD folks might be willing to write a documented specification for this file format? Something like: https://ambermd.org/netcdf/nctraj.xhtml This not only would be a great piece of documentation to your users & the wider community, but it would also make sure that we have a place to look to as the ground truth should you ever decide to change the file format specification. |
I will try to provide a documentation on the code and file format as soon as possible. |
That would be amazing, thank you so much! We would be happy help review the documentation / offer insights if needed. |
There was a problem hiding this comment.
@XiaohuaZhangLLNL this is super cool, more formats are always welcome. FYI you can include a citation to the original source using duecredit like here: https://github.com/MDAnalysis/mdanalysis/blob/develop/package/MDAnalysis/analysis/hydrogenbonds/hbond_autocorrel.py#L217
|
Ah, this sounds like the thing that was developed during the DOE pilot 2 project with Helgi et al. IIRC they were vendoring their own in-house modified version of MDAnalysis at the time. |
|
@tylerjereddy would you be able to act as the PR shepherd here? I am going to assign you but if this doesn't work for you, please unassign yourself and ping me so that I know. Thanks! |
|
sounds good |
There was a problem hiding this comment.
I think the next thing I'm looking for here is an easy way to generate test/sample files--eventually we'll probably want small sample trajectories for testing.
I tried following the instructions at: http://cgmartini.nl/index.php/2021-martini-online-workshop/tutorials/557-10-running-martini-simulation-with-ddcmd#ddcmd-setup
but ran into errors like the one below the fold
Traceback (most recent call last):
File "/home/treddy/github_projects/ddcMDconverter/ddcmdconverter/martini/ITP.py", line 262, in __getattribute__
return self.sections[name]
~~~~~~~~~~~~~^^^^^^
KeyError: 'atomtypes'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/treddy/python_venvs/py_311_mda/bin/martini2obj", line 33, in <module>
sys.exit(load_entry_point('ddcmdconverter', 'console_scripts', 'martini2obj')())
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/treddy/github_projects/ddcMDconverter/ddcmdconverter/martini/martini2obj.py", line 211, in main
for atomtype in par_itp.header.atomtypes.data:
^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/treddy/github_projects/ddcMDconverter/ddcmdconverter/martini/ITP.py", line 264, in __getattribute__
raise AttributeError("{0!r} object has no attribute or section {1!s}".format(self.__class__.__name__, name))
AttributeError: 'Header' object has no attribute or section atomtypes
So, my initial feedback is that it needs to be much easier for me to generate inputs for ddcMD so I can probe for various issues during code review. I do have the GPU-enabled ddcMD binary installed on my system now at least, but the conversion of GROMACS to ddcMD input is causing me too much friction with the current tools, a fast way to really start probing for issues is what I'd like to have, but some sample inputs I can play with directly (without even needing a conversion tool) may be "ok" to start.
| @@ -52,3 +52,5 @@ benchmarks/results | |||
| .idea | |||
| .vscode | |||
| *.lock | |||
| myTest | |||
| myTest2 | |||
There was a problem hiding this comment.
These don't look like .gitignore entries we'd want to keep long term in the project itself.
| elif type[0] == 'f' and bit == 8: | ||
| fieldFmts.append('<d') | ||
| else: | ||
| raise Exception("Add format support for "+type) |
There was a problem hiding this comment.
Can we use an exception class that's a bit more specific than the base one?
| raise Exception("Number of field types doesn't equal to that of field formats") | ||
|
|
||
| if len(fieldTypes) != len(fieldBits): | ||
| raise Exception("Number of field types doesn't equal to that of field bits") |
There was a problem hiding this comment.
Same comment on generic exceptions here and above--a bit more specificity can help for clarity and sane debugging.
There was a problem hiding this comment.
(and in many other places in the diff at the moment..)
|
@XiaohuaZhangLLNL are you able to continue on this PR? If so, it would be helpful if you could respond to @tylerjereddy 's initial review. Thank you! |
Fixes #
Changes made in this Pull Request:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4252.org.readthedocs.build/en/4252/