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

Repair LAMMPS-GAP interface #377

Merged
merged 2 commits into from
Nov 4, 2024
Merged

Repair LAMMPS-GAP interface #377

merged 2 commits into from
Nov 4, 2024

Conversation

abmazitov
Copy link
Contributor

@abmazitov abmazitov commented Nov 4, 2024

This PR fixes the error with LAMMPS-GAP interface resulting in absurd energies of ~ +500000 eV because of the specific format of the LAMMPS outputs (i.e. systems, neghborlists and selected atoms).

Contributor (creator of pull-request) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?
  • Issue referenced (for PRs that solve an issue)?

📚 Documentation preview 📚: https://metatrain--377.org.readthedocs.build/en/377/

Copy link
Contributor

@DavideTisi DavideTisi left a comment

Choose a reason for hiding this comment

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

if passes the test (as it seems it will ) for me it is fine

@DavideTisi DavideTisi merged commit 4b1d1c3 into main Nov 4, 2024
12 checks passed
@DavideTisi DavideTisi deleted the repair-gap-lammps branch November 4, 2024 18:54
@Luthaf
Copy link
Member

Luthaf commented Nov 4, 2024

Sorry, but this is very wrong. Doing it this way means that atoms on the border of domains will have the wrong representation, since their neighbors that belong to other domains will be removed.

This will also create broken NL, containing pairs between atoms that are no longer part of the positions/types arrays.

If you don't understand why some code is written in a given way, please wait more than 15 min before merging change to it ...

@Luthaf
Copy link
Member

Luthaf commented Nov 6, 2024

This PR has been reverted in master, and the original issue will be fixed by metatensor/metatensor#784

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.

3 participants