Raplace the macro with a function

[tjjarvinen]

This PR will implement a new function called AtomsCalculators.complete_interface mean to replace the old macro.

The function works by checking what has been implemented for the given calculator and then implements the missing parts.

Here is in an example a complete energy interface

using AtomsCalculators
import AtomsCalculators: potential_energy
using Unitful
using UnitfulAtomic

struct MyCalc end

AtomsCalculators.energy_unit(::MyCalc) =  u"hartree"
AtomsCalculators.length_unit(::MyCalc) =  u"bohr"

function potential_energy(system, calculator::MyCalc; kwargs...)
       return 0.0u"hartree"
end 

AtomsCalculators.complete_interface(MyCalc)

You can also do this to implement all energy and force calls

using AtomsCalculators
import AtomsCalculators: energy_forces
using Unitful
using UnitfulAtomic

struct MyCalc end

AtomsCalculators.energy_unit(::MyCalc) =  u"hartree"
AtomsCalculators.length_unit(::MyCalc) =  u"bohr"

function energy_forces(system, calculator::MyCalc; kwargs...)
       f = AtomsCalculators.zero_forces(system, calculator)
        return (energy=0.0u"hartree", forces=f)
end 

AtomsCalculators.complete_interface(MyCalc)

In practice you can now just implement whatever and after that call AtomsCalculators.complete_interface, and you will have a complete interface.

The function is now also optimized in a way that it detects optimized calls, like energy_forces forms other optimized calls, like calculate( (Energy(), Forces() ), sys, calc) based on it.

The opposite also works, if there is an optimized calculate( (Energy(), Forces() ), sys, calc) call, then energy_forces call will use that.

implementation_status helper function

There is a new helper function implementation_status that is also useful for problem cases. It basically checks what parts of interface have been implemented and what not

E.g.

using AtomsCalculators
import AtomsCalculators: energy_forces
using Unitful
using UnitfulAtomic

struct MyCalc end

AtomsCalculators.energy_unit(::MyCalc) =  u"hartree"
AtomsCalculators.length_unit(::MyCalc) =  u"bohr"

function energy_forces(system, calculator::MyCalc; kwargs...)
       f = AtomsCalculators.zero_forces(system, calculator)
       return (energy=0.0u"hartree", forces=f)
end 

AtomsCalculators.implementation_status(MyCalc)

returns

Dict{Symbol, Bool} with 11 entries:
  :energy_forces         => 1
  :virial                => 0
  :energy_forces_virial! => 0
  :calculate_virial      => 0
  :energy_forces_virial  => 0
  :calculate_forces      => 0
  :energy_forces!        => 0
  :forces                => 0
  :potential_energy      => 0
  :calculate_energy      => 0
  :forces!               => 0

This function is used by complete_interface function to determine what needs to be implented.`

Discussion and comments

This should now implement all what was discussed in #26 and #27 (@rkurchin and @mfherbst).

In my opinion this is a much better than the macro we use. And I propose we deprecate the macro in favor of this.

The new function should be very robust. It only needs that AtomsCalculators is in scope. After that is should be completely safe to call and it should not give out any errors (other than AtomsCalculators has not been defined). In fact you can call the function several times and it would not do anything extra. You can also call the function in a different module and it should still complete the interface.

Documentation page is still missing. I will add it, if we agree that this function is the way to go.

While the new function is much more simpler to use and less error prone, it is actually a lot more complicated that the old macro. This is a possible error source. But, I believe I managed to iron out the errors. The issue is that there are so many possible implementation combinations that it is not possible to test all of them. How ever, in practice only couple of all possibilities are used in practice. So this should be fine.

[mfherbst]

In my opinion this is a much better than the macro we use. And I propose we deprecate the macro in favor of this.

I agree with the deprecation. I'm still a little unsure about this function, because it may be too magical and clever.

Of course such a function is great if it works, but it has a chance of hiding the details too much from the developer, such that in weird edge cases one still needs to implement everything manually (think of an odd calculator where the best dispatch path just happens to be different than the one you have implemented for a weird reason). I think too magical functions tend to increase the chance for bugs and technical debt, which may be hard to detect and fix in the long run.

My main gripe is that it is a lot of code to make an automated decision, which could instead be put in the responsibility of the developer calling this function. If they are forced to specify what kind of things they want implemented, then the mechanics of this function is more visible and explicit, which also means that a developer can choose to not do one thing for whatever reason, without missing out of all the convenience of the function.

[mfherbst]

This is a lot of code. Can this not be simplified by taking some flags (symbols) which the developer has to specify ? This makes this function shorter, easier to understand what it does and its action more explicit. I am almost sure this will avoid bugs in the long run.

[mfherbst]

Looks like unintended commit.