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

WIP: Move generic site potential codes from EP #7

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

Conversation

cortner
Copy link
Member

@cortner cortner commented May 24, 2024

This PR moves the generic site potential utilities from EP to this new utilities package.

  • move over all generic codes
  • move over hessian codes (and close the corresponding EP PR)
  • Fix usage of the @generate_interface macro, I keep getting errors with that
  • Discuss and finalize how we handle units
  • Ensure 100% compliance with AtomsCalculators, the only question is compliance with current interface or the one being developed?

TODO for subsequent PRs

  • switch to an in-place interface for the site calculations and integrate Bumper
  • move the interface to the low-level parameterized interface + provide wrappers for the high-level interface?
  • implement pullbacks through energy, forces, virials (this can be moved over from the ACEpotentials.jl prototype)

@cortner
Copy link
Member Author

cortner commented May 25, 2024

@tjjarvinen -- Two questions

(1) should the pair implementation stay in EP or move to ACU? My intuition is to move it to ACU, but I don't have too strong a view.

(2) If moving to ACU, would you like to make that move since you implemented the pair potentails in the first place? You can directly amend the current PR or branch off of it - whichever you prefer.

If I were to make the move, then I would re-implement it quite differently by accessing the neighbourlist directly, never computing the site energies but directly the summation over pairs. I would also bring it in line with my slightly changed way to handle units for site potentials.

There is also no immediate rush so we can first meet in person next week and discuss all the next steps before deciding.

@tjjarvinen
Copy link
Collaborator

I have no strong feeling about pair potential location either. If we move it ACU, then it would be the only actual calculator in ACU. So, I would kinda prefer no not move it there and dedicate ACU only to utilities for actual calculators. But this is a very mild preference.

There is also the possibility to use CellListMap for pairpotentials in a long term. So, it might be a good point to wait a little bit and see how things develop with neighbourlists. But, as you said it is probably better discuss more in person.

@cortner
Copy link
Member Author

cortner commented May 26, 2024

There is also the possibility to use CellListMap for pairpotentials in a long term

I would very much like an interface that selects either NeighbourLists or CellListMap depending whether the min-image conventin is satisfied. At the moment you cannot use CellListMap with pair potentials when the computational cell is small and has PBC.

@cortner
Copy link
Member Author

cortner commented May 27, 2024

Here is a small argument to move a GENERIC pair potential implementation - not a concrete one - into ACU: I would like to use such a generic implementation in ACEpotentials and possibly elsewhere. But I don't see why ACEpotentials should depend on EP.

EP should still have specific pair potentials such as Morse, LJ, ZBL etc. (though with ZBL the case is also less clear - it is not a model by itself, just a model component ...)

@tjjarvinen
Copy link
Collaborator

That is a good argument and we should go for it.

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.

None yet

2 participants