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.

Sign up for GitHub

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

Jump to bottom

LAMMPS non-unique molecule ID hotfix #1000

Merged
merged 18 commits into from
Jul 16, 2024
Merged

LAMMPS non-unique molecule ID hotfix #1000

merged 18 commits into from
Jul 16, 2024

Conversation

timbernat
Copy link
Contributor

@timbernat timbernat commented Jul 9, 2024 •
edited
Loading

Description

Follow-up to recommendations made in Issue 999

Checklist

  • Add molecule_index to CTAB hash to ensure uniqueness
  • Add tests
  • Ensure tests are compliant with pytest
  • Lint
  • Update docstrings

@timbernat timbernat added bug Something isn't working lammps Relating to LAMMPS labels Jul 9, 2024
@timbernat timbernat self-assigned this Jul 9, 2024
@timbernat timbernat mentioned this pull request Jul 9, 2024
Closed
3 tasks
@mattwthompson mattwthompson mentioned this pull request Jul 9, 2024
Open
mattwthompson
mattwthompson reviewed Jul 9, 2024
View reviewed changes
Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

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

Looking good, I have a few comments that might streamline some of the stuff you're doing here. Interoperability work is unfortunately a catastrophic mess of complexity, so "unit" tests can't be the beautiful <10 LOC tests they teach in textbooks.

openff/interchange/interop/lammps/export/export.py Outdated Show resolved Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py Outdated Show resolved Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py Outdated Show resolved Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py Outdated Show resolved Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py Outdated Show resolved Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py Outdated Show resolved Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py Outdated Show resolved Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py Outdated Show resolved Hide resolved
timbernat and others added 13 commits July 15, 2024 18:09
@timbernat @mattwthompson
Apply suggestions from code review
d988cbf 
Incorporated suggestions for unique mol ID unit test improvements

Co-authored-by: Matt Thompson <[email protected]>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
2e3de08 
for more information, see https://pre-commit.ci
@timbernat
Removed unnecessary typing imports
424de3b
@timbernat
Consolidated pilot_mol init and conformer generation using MoleculeWi…
4c50cd3 
…thConformer
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
46505da 
for more information, see https://pre-commit.ci
@timbernat
Updated sidelen default, added assertion to enforce test for multiple…
8c1fc7b 
… Molecules
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
99d3fdf 
for more information, see https://pre-commit.ci
@timbernat
Wrapped mol ID + CTAB hash with hash() to produce more literal value …
5266a28 
…for molecule_hash
@timbernat
Re-implemented LAMMPS file molecule ID extraction with in-Python lamm…
8b72acd 
…ps interface
@timbernat
Merge remote-tracking branch 'refs/remotes/origin/lammps-hotfix' into…
5d49deb 
… lammps-hotfix
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
e3f1d27 
for more information, see https://pre-commit.ci
@timbernat
Added pytest parametrization of auxiliary args in test_unique_lammps_…
652ad43 
…mol_ids()
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
e4ffb11 
for more information, see https://pre-commit.ci
@timbernat
Copy link
Contributor Author

timbernat commented Jul 16, 2024 •
edited
Loading

Alright @mattwthompson, I've incorporated the streamlining suggestions you made in one form or another and verified that the test still behaves as expected. Could you have a final look through to see if we're ready to merge?

@codecov Codecov
Copy link

codecov bot commented Jul 16, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.39%. Comparing base (65f36d5) to head (2911b38).
Report is 2 commits behind head on main.

Additional details and impacted files

@mattwthompson
Copy link
Member

@timbernat Thanks for your patience on this; since the actual change is fairly small I'm just going to pull the trigger

If there are other changes that would make your work run smoother, please feel free to coordinate with me on them or open PRs directly. Stuff in lammps-improvements was probably well-intentioned but lost attention in the mess of things

@mattwthompson mattwthompson merged commit 2a4c999 into main Jul 16, 2024
22 checks passed
@timbernat timbernat deleted the lammps-hotfix branch July 16, 2024 18:00
@timbernat
Copy link
Contributor Author

Thanks so much for all the help, @mattwthompson! I've deleted the lammps-hotfix branch and closed PR #999, since we've resolved that here. I'll be sure to loop you in on any proposed LAMMPS-layer improvements as they come up!

A final question, what should be the fate of the lammps-improvements branch (335 commits behind main, at time of writing)? I'll make new branches for any future changes I propose, but the dangling orphan branch makes me a little uneasy. Is it worth deleting, pulling into from main, or something else entirely?

@mattwthompson
Copy link
Member

Let me try to revive it and see if it needs a rebase or something funkier

@mattwthompson
Copy link
Member

It looks like that branch wasn't so out of date it couldn't have main merged back in. Have a look and let's go from there: #1011

@mattwthompson mattwthompson mentioned this pull request Jul 16, 2024
Closed
@PEFrankel PEFrankel mentioned this pull request Jul 17, 2024
Closed
3 tasks
@timbernat timbernat mentioned this pull request Aug 23, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattwthompson mattwthompson mattwthompson left review comments

Assignees

@timbernat timbernat

Labels
bug Something isn't working lammps Relating to LAMMPS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timbernat @mattwthompson