mdanalysis icon indicating copy to clipboard operation
mdanalysis copied to clipboard

Issue 3449 lammps dump parser

Open amirhs1 opened this issue 3 years ago • 10 comments

Fixes #

Changes made in this Pull Request:

  • LAMMPS standard atom styles are added to DATAParser, so it can now handle all LAMMPS atom styles as listed in read_data and atom_style commands. Consequently, the former freedom in passing a user-define atom style via atom_style kwarg is replaced with replaced with a list of pre-defined atom styles. Accordingly, all the relevant parsing methods are changed.
  • The header dictionary is passed as a new argument to all parsing method for satiny check. Now, each parsing such as _parse_masses or _parse_pos check whether the length of datalines is equal to a corresponding item in header dictionary or not.
  • All the field(s) words in the documentations or comments changed to attribute(s) since the later is the correct physical/technical term used in LAMMPS or MDAnalysis for referring to a physical attributes of an atom.
  • Some cosmetic changes done; for instance, unused imports deleted.

Note

  • This pull request does not address #3449 directly, but it generalizes the process of parsing LAMMPS topology information from DATA an/or DUMP files.
  • A pull for modifying LammpsDumpParser is request shortly after this one.

PR Checklist

  • [ ] Tests?
  • [x] Docs?
  • [ ] CHANGELOG updated?
  • [ ] Issue raised/referenced?

amirhs1 avatar Apr 17 '22 18:04 amirhs1

Hello @amirhs1! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2022-06-14 13:46:20 UTC

pep8speaks avatar Apr 17 '22 18:04 pep8speaks

Hi @amirhs1 you seem to have added a bunch of files to the diff (and removed some) that you did not intend to (29 files changed). Please fix this up so only the right files appear in the diff before we go on to review your PR :)

hmacdope avatar Apr 18 '22 10:04 hmacdope

Hi Hugo!

I did not delete or change any other file personally, but let me check. Is it possible that a cloud service such iCloud added some hidden files to the repo directory on my machine? And then git commit those files?

Amir

On Apr 18, 2022, at 06:05, Hugo MacDermott-Opeskin @.***> wrote:

 Hi @amirhs1 you seem to have added a bunch of files to the diff (and removed some) that you did not intend to (29 files changed). Please fix this up so only the right files appear in the diff before we go on to review your PR :)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

amirhs1 avatar Apr 18 '22 10:04 amirhs1

Did you use git commit * at any point ? That can sometimes do it. Have a look in the Files Changed tab in this PR.

hmacdope avatar Apr 18 '22 10:04 hmacdope

Did you use git commit * at any point ? That can sometimes do it. Have a look in the Files Changed tab in this PR.

No! Let me check.

amirhs1 avatar Apr 18 '22 11:04 amirhs1

@hmacdope is it ok now?

amirhs1 avatar Apr 18 '22 13:04 amirhs1

@amirhs1 yep the diff looks good now, I'll review as soon as I can.

hmacdope avatar Apr 19 '22 00:04 hmacdope

In my pull request, I limited the functionality of the atom_style kwarg, so it only accepts LAMMPS standard data formats. However, this limitation can be lifted and the atom_style can also accept user-defined styles. What do you think? In the case of LAMMPSDumpParser, I did not add an atom_style kwarg; instead, the code itself infers the column names for the information provided in the LAMMPSDUMP file. This also can be changed, so a atom_style kwarg can be defined that accepts both the user-defined styles and LAMMPS standard dump styles. Again, what do you think?

amirhs1 avatar Apr 19 '22 12:04 amirhs1

Sorry for my long absence! I was on a vacation for 5 weeks!

amirhs1 avatar Jun 03 '22 21:06 amirhs1

@amirhs1 very jealous. Ping me if you need review.

hmacdope avatar Jun 07 '22 00:06 hmacdope