Fix and enhance lammps DATAWriter #4520
Conversation
|
Hello @nbzy1995! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
![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.
Linter Bot Results:Hi @nbzy1995! 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4520 +/- ##
===========================================
- Coverage 93.65% 93.62% -0.04%
===========================================
Files 168 180 +12
Lines 21215 22306 +1091
Branches 3908 3913 +5
===========================================
+ Hits 19869 20883 +1014
- Misses 888 965 +77
Partials 458 458 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Great start @nbzy1995.
Would you be able to write up an issue that this PR addresses? There are at least 25 different atom_style variants and we need to ensure this solves an issue that is likely to be useful to the community (yourself included of course) without taking on functionality we cannot maintain.
| @@ -274,46 +275,63 @@ def __init__(self, filename, convert_units=True, **kwargs): | |||
| self.units['velocity'] = kwargs.pop('velocityunit', | |||
| self.units['length']+'/'+self.units['time']) | |||
|
|
|||
| def _write_atoms(self, atoms, data): | |||
| def _write_atoms(self, atoms, data, atom_style): | |||
There was a problem hiding this comment.
Forgive my LAMMPs ignorance, but what style are we currently writing? We should use that as the default.
|
|
||
| indices = atoms.indices + 1 | ||
| types = atoms.types.astype(np.int32) | ||
|
|
||
| moltags = data.get("molecule_tag", np.zeros(len(atoms), dtype=int)) | ||
| if atom_style == 'full': |
There was a problem hiding this comment.
Need to test for valid atom styles, I would add them as a static list object somewhere in this file. e.g
_allowed_atom_styles = ["full, atomic, ..."]
if atom_style not in _allowed_atom_styles:
raise ValueError("blah")|
|
||
| if has_charges: | ||
| if has_charges and print_moltag: |
There was a problem hiding this comment.
It may be better to separate these by atom_style it is not very clear to which style these branches belong (should use a case statement). Not really your fault but will make things a lot clearer.
| @@ -230,14 +230,14 @@ def test_datawriter_universe(filename, tmpdir): | |||
| """ | |||
| fn = str(tmpdir.join(filename)) | |||
|
|
|||
| u = mda.Universe(LAMMPSdata_mini) | |||
| u = mda.Universe(LAMMPSdata_mini, convert_units = False) # By defaul convert_units = True | |||
There was a problem hiding this comment.
Parametrize over convert_units = False and convert_units = False with @pytest.mark.parametrize
| assert_allclose(u.atoms.positions, u2.atoms.positions) | ||
| assert_allclose(u.atoms.velocities, u2.atoms.velocities) | ||
| assert_allclose(u.dimensions, u2.dimensions) | ||
|
|
||
|
|
There was a problem hiding this comment.
Can you roundtrip one of the files we write to ensure it is readable?
|
@hmacdope could you please be in charge? This may simply mean closing the PR as stale if there's no reply to your questions and requests. |
Fixes #
Changes made in this Pull Request:
convert_units = FalseinDATAWriterinlammps.pyDATAWriterinlammps.pyto use differntatom_stylePR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4520.org.readthedocs.build/en/4520/