Bug with low-level interface and generate_interface macro

[mfherbst]

I am in the process of updating ASEconvert to v0.2 (https://github.com/mfherbst/ASEconvert.jl/pull/21)

This works:

import AtomsCalculators: @generate_interface

@generate_interface function AtomsCalculators.calculate(::AtomsCalculators.Energy,
        system, calc::ASEcalculator, parameters=nothing, state=nothing; kwargs...)
    # ...
end

but this does not

import AtomsCalculators: @generate_interface, calculate, Energy, Forces, Virial

@generate_interface function calculate(::Energy,
        system, calc::ASEcalculator, parameters=nothing, state=nothing; kwargs...)
    # ...
end

and (related to #23) gives the rather cryptic error message

Possible typo (or other issue) in function definition. Could not determine the type of calculation (energy, forces or virial...).

from which it took me a while to find out what I had to change to the function definition to make the macro work.

@tjjarvinen Could you take a look?

[tjjarvinen]

I got this

import AtomsCalculators: @generate_interface, calculate, Energy, Forces, Virial
abstract type ASEcalculator end

@generate_interface function calculate(::Energy,
               system, calc::ASEcalculator, parameters=nothing, state=nothing; kwargs...)
           # ...
       end

Giving this

ERROR: LoadError: Possible typo (or other issue) in function definition. Could not determine the type of calculation (energy, forces or virial...).
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] var"@generate_interface"(__source__::LineNumberNode, __module__::Module, expr::Any)
   @ AtomsCalculators ~/.julia/packages/AtomsCalculators/pKMDb/src/utils.jl:43
in expression starting at REPL[10]:1

caused by: type Symbol has no field head
Stacktrace:
 [1] getproperty(x::Symbol, f::Symbol)
   @ Base ./Base.jl:37
 [2] determine_type_calculation(expr::Expr)
   @ AtomsCalculators ~/.julia/packages/AtomsCalculators/pKMDb/src/utils.jl:114
 [3] var"@generate_interface"(__source__::LineNumberNode, __module__::Module, expr::Any)
   @ AtomsCalculators ~/.julia/packages/AtomsCalculators/pKMDb/src/utils.jl:41

Line 114 in utils reads

if expr.args[1].args[1].head != :.
     error("function definition is not correct")
end

This part will check that the call is AtomsCalculators.calculate, if you call with calculate only the check will fail.

So, calling

@generate_interface function AtomsCalculators.calculate(::Energy,
               system, calc::ASEcalculator, parameters=nothing, state=nothing; kwargs...)
           # ...
       end

works.

As a note. In the initial draft for AtomsCalculators there was discussion of wheter the definition need to be AtomsCalculators.calculate or would simply calculate be enough. The conclusion was that because we don't export anything wrom AtomsCalculators the definitions need to be in form AtomsCaluculators.calculate in order to work.

That being said the error message here should be better. Also the documentation should explain better the macro use.

[mfherbst]

That being said the error message here should be better. Also the documentation should explain better the macro use.

I agree. In any case I would argue that it is confusing to standard Julia programmers that extending the function AtomsCalculators.calculate using the macro only works if one uses the function definition of the form function AtomsCalculators.calculate( ... ) and not function calculate( ... ). So we need to make sure either both things work or a future user will see this before trying the shorter version.

[rkurchin]

I ran into the same error message (albeit for different reasons) during the JuliaCon hackathon when trying to get Cedric's branch to work (see https://github.com/CedricTravelletti/DFTK.jl/pull/4, which I think might be unnecessary now so I can probably close it?) awhile back, so seconded that this error could be better. I'm not so experienced with some of this metaprogramming stuff, but it seems that one should be able to write a more flexible/less hardcoded kind of check than what's there now...

[tjjarvinen]

Here is a little bit more on the functionality of the macro.

You can do following just fine

using Unitful
import AtomsCalculators: @generate_interface, calculate, Energy, Forces, Virial

abstract type MyCalc end

function calculate(::Energy, sys, calc::MyCalc, pr=nothing, st=nothing; kwargs...)
      ## ....
     return 0.0u"eV"
end

You then would need to generate the high-level interface. The macro in practice wraps the low level call to following

function AtomsCalculators.potential_energy(sys, calc::MyCalc, kwargs...)
   tmp = AtomsCalculators.calculate(AtomsCalculators.Energy(), sys, calc::MyCalc, nothing, nothing; kwargs...)
   return tmp.energy
end

If you run that code in the same context

using Unitful
import AtomsCalculators: @generate_interface, calculate, Energy, Forces, Virial

abstract type MyCalc end

function calculate(::Energy, sys, calc::MyCalc, pr=nothing, st=nothing; kwargs...)
      ## ....
     return 0.0u"eV"
end

function AtomsCalculators.potential_energy(sys, calc::MyCalc, kwargs...)
   tmp = AtomsCalculators.calculate(AtomsCalculators.Energy(), sys, calc::MyCalc, nothing, nothing; kwargs...)
   return tmp.energy
end

this will fail because AtomsCalculators or potential_energy are not in the scope. This is the main issue with the macro. Because you generate function calls you need to make sure the functions you extend are in the scope. The easies way to do this is to make sure that AtomsCalculators is in the scope. Then you can use AtomsCalculators.potential_energy etc.

We could have all the individual functions in the scope, but that would pollute the scope, and we want to keep it small as possible also. Thus the best option is to have only AtomsCalculators in scope that allows extending everything with AtomsCalculators.potential_energy type calls.

Currently the macro checks that AtomsCalculators is in scope by checking that the function was called with AtomsCalculators.calculate way. We could move the check to be of type "is AtomsCalculators in scope` then you could do following

using AtomsCalculators  # or import AtomsCalculators
import AtomsCalculators: @generate_interface, calculate, Energy, Forces, Virial

@generate_interface function calculate(::Energy,
        system, calc::ASEcalculator, parameters=nothing, state=nothing; kwargs...)
    # ...
end

This way we could have better error messages also.

[rkurchin]

I'm definitely in favor of anything that facilitates better error messages!

[mfherbst]

I also like your last suggestion @tjjarvinen.

Also related to #27, I am wondering if instead of annotating functions with a macro, a simpler solution could be to have two generate macros, one for the high-level implementation pattern and one for the low-level implementation pattern. My idea is that you define all methods you want to specialise and then call @generate_interface_highlevel or @generate_interface_lowlevel to just add the remaining functions to do the fallback routing. In that case the macros don't have to be so smart (which likely leads to a more stable implementation and better error messages).

[tjjarvinen]

Yes we can do that.

After giving it a little though, this will be a better way and I can add other functionality too. I will make a PR for it.

Thanks for the idea!