Skip to content

Remove hybrid styles from LAMMPS export #1203

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

benjamindjensen
Copy link
Contributor

Description

One of the changes identified in #955 is removing hybrid styles from the LAMMPS export. Hybrid styles are not currently required for OpenFF potentials. Including hybrid LAMMPS potentials in a data file is inconvenient for users because LAMMPS will not write hybrid coefficients to restart/data files for subsequent simulations, requiring a user to manually implement the coefficients in the LAMMPS script instead of the data file. If OpenFF requires hybrid styles in the future (using both LJ and Buckingham potentials for example) I would recommend providing the force field coefficients in the LAMMPS script instead of the data file.

Checklist

  • Add tests
  • [ X] Lint
  • Update docstrings

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@mattwthompson
Copy link
Member

Heh, I forget exactly why I went with "hybrid" potentials but at the time I thought it was the only way to get some detail correct. I'd be happy to be wrong - I never used them when I was running simulations myself.

Could you drop the openeye: true lines from the workflows and see if they run?

Copy link

codecov bot commented Apr 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.31%. Comparing base (a3378ee) to head (428757c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1203      +/-   ##
==========================================
- Coverage   93.60%   93.31%   -0.30%     
==========================================
  Files          70       70              
  Lines        6039     6039              
==========================================
- Hits         5653     5635      -18     
- Misses        386      404      +18     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@benjamindjensen
Copy link
Contributor Author

benjamindjensen commented Apr 18, 2025

I'm not particularly familiar with github actions. I modified the ci.yaml file and it seems to have passed most of the tests, although a few are failing from what looks like an openeye license error. Let me know if there's a more appropriate way to modify the workflow for this pull.

I've also not used hybrid potentials in lammps, but have been using this modification myself for a while now and thought it would be helpful for others to have in the official repository.

@mattwthompson
Copy link
Member

@benjamindjensen what version of LAMMPS are you using? What version(s) do you expect this to work with?

@mattwthompson
Copy link
Member

Nevermind, I was making an error elsewhere in my tooling.

I've grabbed these changes locally and everything looks good. I'm opening this in a separate PR for boring reasons relating to our automation, but I think it's good to go.

I assume that @mrshirts and @timbernat are okay with this change?

@mattwthompson mattwthompson mentioned this pull request Apr 21, 2025
3 tasks
@benjamindjensen
Copy link
Contributor Author

Nevermind, I was making an error elsewhere in my tooling.

I've grabbed these changes locally and everything looks good. I'm opening this in a separate PR for boring reasons relating to our automation, but I think it's good to go.

I assume that @mrshirts and @timbernat are okay with this change?

Great! Thank you for including this in Interchange.

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.

2 participants