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

Lammps charmm dihedrals #787

Merged
merged 30 commits into from
Jan 30, 2024
Merged

Conversation

CalCraven
Copy link
Contributor

No description provided.

gmso/utils/compatibility.py Fixed Show fixed Hide fixed
gmso/utils/compatibility.py Fixed Show fixed Hide fixed
gmso/formats/lammpsdata.py Fixed Show fixed Hide fixed
@CalCraven CalCraven added the WIP work in progress - do not merge label Nov 27, 2023
gmso/formats/lammpsdata.py Fixed Show fixed Hide fixed
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (fce6f42) 92.36% compared to head (11bbf15) 92.42%.

Files Patch % Lines
gmso/utils/compatibility.py 79.16% 5 Missing ⚠️
gmso/formats/lammpsdata.py 98.76% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #787      +/-   ##
==========================================
+ Coverage   92.36%   92.42%   +0.06%     
==========================================
  Files          66       66              
  Lines        6864     7024     +160     
==========================================
+ Hits         6340     6492     +152     
- Misses        524      532       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CalCraven
Copy link
Contributor Author

Noting two issues still to deal with:

  1. We need to make sure that we want this way to deal with potentials that have terms, such as charmm dihedrals and cvff params that need to be given multiple lines for the same dihedal types
  2. There is a lot of optimization to be done, this still takes on the order of 15 minutes for 1000 atoms, which doesn't scale great.

gmso/formats/lammpsdata.py Fixed Show fixed Hide fixed
gmso/formats/lammpsdata.py Fixed Show fixed Hide fixed
…to iterate through topology attributes with different potential filters
Copy link
Member

@daico007 daico007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got some comments, but I will need to come back to this later this week, still probably have a bit of the lammps writer (the smaller utilities).

gmso/tests/parameterization/test_molecule_utils.py Outdated Show resolved Hide resolved
gmso/tests/parameterization/test_molecule_utils.py Outdated Show resolved Hide resolved
gmso/utils/compatibility.py Outdated Show resolved Hide resolved
gmso/utils/compatibility.py Outdated Show resolved Hide resolved
gmso/utils/compatibility.py Show resolved Hide resolved
gmso/formats/lammpsdata.py Outdated Show resolved Hide resolved
gmso/formats/lammpsdata.py Outdated Show resolved Hide resolved
gmso/formats/lammpsdata.py Outdated Show resolved Hide resolved
gmso/formats/lammpsdata.py Outdated Show resolved Hide resolved
gmso/formats/lammpsdata.py Outdated Show resolved Hide resolved
@daico007
Copy link
Member

I think I went through all the changes (and some local tests). Most of the logic is sound, only some minor suggestion with code styling. The only issue I think we need to resolve is with the priority of grabbing value for site.charge (see comment above).

@CalCraven CalCraven removed the WIP work in progress - do not merge label Jan 24, 2024
@CalCraven
Copy link
Contributor Author

All comments look great @daico007. Will incorporate these tomorrow. Agreed about some of the messiness of those loops. I'll see if I can think of a cleaner way to write it, because even I look at it and my eyes glaze over.

Copy link
Member

@daico007 daico007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Will merge once all tests pass.

@daico007 daico007 merged commit 7c97931 into mosdef-hub:main Jan 30, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants