Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alternative lammps parse #658

Merged
merged 4 commits into from
Nov 7, 2022
Merged

Alternative lammps parse #658

merged 4 commits into from
Nov 7, 2022

Conversation

Leimeroth
Copy link
Member

@Leimeroth Leimeroth commented Jun 22, 2022

Another approach to parsing without readlines() and multiple loops

EDIT (@liamhuber -- correct if I'm wrong about this, Niklas): Intended to solve #671

@coveralls
Copy link

coveralls commented Jun 22, 2022

Pull Request Test Coverage Report for Build 3095441284

  • 16 of 17 (94.12%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 68.274%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_atomistics/lammps/base.py 16 17 94.12%
Totals Coverage Status
Change from base Build 3089757525: 0.02%
Covered Lines: 12090
Relevant Lines: 17708

💛 - Coveralls

@pmrv
Copy link
Contributor

pmrv commented Jun 22, 2022

Based on this I've found a version that is even more efficient, will push it when I have cleaned it up. In the meantime feel free to merge #657, so you don't need to wait for me.

@jan-janssen
Copy link
Member

Using a for loop could be an issue performance wise, is it possible to solve this with arrays as @samwaseda did before?

@Leimeroth
Copy link
Member Author

I am not sure what @samwaseda did, but the previous implementation read in the whole file into memory and then did multiple list comprehensions and initialized seperate StringIO and DataFrame instances for each line, so I assume this is a improvement already.
Anyway @pmrv did some tests and already found a better version so this was not merged yet, but only exists as a reminder.

@Leimeroth
Copy link
Member Author

@pmrv should I merge this or do you want to create a PR for your version?

@jan-janssen
Copy link
Member

@pmrv should I merge this or do you want to create a PR for your version?

Can we still do a set of benchmarks before we merge these changes? I have the feeling we are moving back and forth with the LAMMPS parser, so I would like to have clear reasoning why we choose one implementation over another.

@stale
Copy link

stale bot commented Aug 11, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 11, 2022
@Leimeroth Leimeroth removed the stale label Aug 12, 2022
@stale
Copy link

stale bot commented Sep 21, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 21, 2022
@stale stale bot removed the stale label Sep 21, 2022
@stale
Copy link

stale bot commented Oct 15, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 15, 2022
@Leimeroth
Copy link
Member Author

up

@stale stale bot removed the stale label Oct 17, 2022
@stale
Copy link

stale bot commented Nov 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 1, 2022
@pmrv
Copy link
Contributor

pmrv commented Nov 5, 2022

At this point, let's just merge this and revisit with pymatgen parser at some point in the future.

@stale stale bot removed the stale label Nov 5, 2022
@Leimeroth Leimeroth merged commit dfd0330 into master Nov 7, 2022
@delete-merged-branch delete-merged-branch bot deleted the alternative-lammps-parse branch November 7, 2022 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants