JuliaMolSim / AtomsCalculatorsUtilities.jl

Utilities for implementing AtomsCalculators
MIT License
2 stars 1 forks source link

WIP: Move generic site potential codes from EP #7

Closed cortner closed 3 months ago

cortner commented 5 months ago

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

TODO for subsequent PRs

cortner commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

cortner commented 3 months ago

closing this in favour of the new PR that incorporates the move to ACE 0.2 #18

cortner commented 3 months ago

NB the two open items seem to have been resolved in AC0.2 and so just updating to AC0.2 should resolve them.