Rethink units

[cortner]

• should the site potential inner evaluator determine the units or be unitless? i.e. what does eval_site return?
• at the moment eval_site should be unitless. But then why is cutoff_radius assumed to return a distance unit?
• (mostly style) the units for energy are applied outside the loop, the units for force inside the loop. why?

[tjjarvinen]

• cutoff_radius is an input to potential definition and it has no meaning without unit. We could fix the unit for the calculation, but it is better to allow some control on what lenght units the calculation uses.
• forces are accumulated during the calculation to force array, which is allocated with AtomsCalculators.zero_forces call that has by definition units in it. Calling AtomsCalculators.forces! thus will always have units in force array. So, this is mainly to make it nice with AtomsCalculators. Energy is just a one number. It is better to only put the unit at the end, as that minimizes the needed operations on it.

[cortner]

This is not nearly as obvious as you are making it sound. Right now it is highly inconsistent which makes it confusing to read the code and very hard to implement a new calculator.

E.g. we can define distance_unit(calc) but have cutoff_radius(calc) return just a unit-less number. that would be consistent with how we compute site energies and their gradients.

Secondly, the cost of applying the unit inside the loop to the energy will not make a measurable difference. For readability otoh both need to be applied inside or both need to be applied outside.

[cortner]

I now also notice that in some assembly loops the force units are applied outside as well. So the whole thing is inconsistent which makes it harder to maintain.

[tjjarvinen]

The loop where force units are applied outside it the one with parameter estimations that uses ForwardDiff that caused some issues with units and it was better to applie them outside. That being said the parameter estimation part is a bit experimental still and we need to give it a second look at some point.

[cortner]

At the moment the following seems to work of for me. It still feels like a hack, but maybe it is ok:

• enforce the site potential to be unit-free i.e. eval_site etc work without units (I don't like the force_unit or the one(calc) those feel even more hacky)
• implement energy_unit(calc) and length_unit(calc) => from these we can derive all others.
• the outer assembly does an @assert to check that the input vectors (Rs and bb) have the correct length unit. Or alternatively converts them to the unit that the inner site potential expects.

This is a relatively small modification of your current framework and probably overall consistent with what you have in mind?

[cortner]

move discussion to AtomsCalculatorsUtilities