PairPotentials for Multiple Elements

[cortner]

Currently, the pair potential is implemented in a very restrictive way, allowing only 2 atom ids. All other pairs are ignored. This requires stacking a large number of calculators to get a general pair potential for multiple elements. The current approach is still valuable if one actually wants a potential that acts only on two elements out of many possibilities. However, I consider this a corner case rather than the typical case.

I normally implement this differently, e.g. with splined pair pots as follows

struct SplinePairPot{NZ, SPLINE, NZSQ} 
     zlist::NTuple{NZ, AtomID}  
     splines::SMatrix{NZ, NZ, SPLINE, NZSQ} 
end

Then in the assembly of energies it becomes something like

      E += pot.splines[iz0, iz1](r)

Both approaches have advantages and disadvantages. E.g. the second approach becomes problematic when the splines are replaced by functions since one then has to use function-wrappers to avoid type instabilities - also doable but an additional burden.

I think it is possible to support both approaches and I would like to try and do that. This issue is primarily to discuss terminology, i.e. the names of the different types and to discuss whether it is worth having separate implementations since the currently implemented version is actually a special case.

[cortner]

Assuming we wish to keep separate implementations of the two approaches, I propose the following type hierarchy and names...

abstract type AbstractPairPotential <: SitePotential end 

# I suggest to rename the current implementations to 
struct FilterPairPotential{TFUN, ID, TC} <: AbstractPairPotential
   f::TFUN
   atom_ids::NTuple{2,ID}
   cutoff::TC
end

# The new pair potential could be called 
struct PairPotential{NZ, ID, TC, TFUN, NZSQ} <: AbstractPairPotential 
      zlist::NTuple{NZ, ID}
      cutoff::TC 
      f::SMatrix{NZ, NZ, TFUN, NZSQ}
end

I think the parameterized versions do not need to be implemented separately. I think they can be quite easily merged into a single implementation; see separate issue.

An alternative approach might be to do something like

struct PairPotential{NZ2, ID} <: AbstractPairPotential 
      zpairs::NTuple{NZ2, Tuple{ID, ID}}
      f::NTuple{NZ2, TFUN}
      cutoff::TC 
end

This would cover both cases, but would require a fast lookup into zpairs. If that list becomes long, this may become problematic. But I am uncertain about it.

[cortner]

@tjjarvinen -- since you implemented the current version of pair potentials, your thoughts would be useful.

[tjjarvinen]

I would go with the two different implementation one.

There is a performance penalty with parametric one over non-parametric, so I would atleast check is it significant after neighbourlist update. Benchmark has this covered, and iirc the difference with current implementation is one order of magnitude, but that could be due to some type instability issues.

[cortner]

Surely the performance penalty is paid by the non-parametric potential since it has a type instability.thats exactly my complaint in #12

[cortner]

I think this issue is now irrelevant - the interface proposed in #14 and implemented in #18 allows all options to be implemented.