Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making use of state to accelerate calls like energy_forces or calculate((Energy(), Forces()), ...) in default implementation #27

Open
mfherbst opened this issue Jul 28, 2024 · 2 comments

Comments

@mfherbst
Copy link
Member

The default implementation of calls like energy_forces or calculate((Energy(), Forces()) do not allow us to make use of a state in DFTK to reduce the computational burden on the second call (force computation). This should be done in the default implementation automatically. Essentially I suggest:

  • Calls like energy_forces should dispatch to the low-level interface (calculate((Energy(), Forces()) )
  • The implementation of calculate(::Tuple ) should transfer the state when dealing with one tuple element to the calculate call when dealing with the next.
@tjjarvinen
Copy link
Collaborator

The default implementation of calculate should already transfer state for following call

function calculate(properties::Tuple, sys, calc, params=nothing, state=nothing; kwargs...)
    out = Dict{Symbol, Any}()
    out[:state] = state
    for property in properties
        tmp = calculate(property, sys, calc, params, out[:state]; kwargs...)
        for (k,v) in pairs(tmp)
            out[k] = v
        end
    end
    return NamedTuple(out)
end

If that does not happen in practice, it is a bug.

The combination call energy_forces etc. issue is a bit problematic. It depends on whether you implement low or high livel interface.

For energy_forces this is not an issue, but for energy_forces_virial it can become an issue. If we implement both of these calls to call calculate. The calls would work for low level implementation. High level implementation on the other hand would need to implement both energy_forcers and energy_forces_virial calls, and then on top of that calculate calls also.

I currently don't know how to solve this "nicely". One option would be to have the macro cover this also, but I am not sure can it be done.

In any case, I know this is an issue and I have been thinking about it, but I don't have solution to it yet. So, at this moment, if you implemet calls that optimise combination calls, it is up to you to implement them.

@mfherbst
Copy link
Member Author

Indeed it does work in the current version, sorry. I think I looked at old code.

Regarding what you mention with the high versus low level issue. Would it not work to have a macro to generate appropriate fallback functions for the low-level implementation pattern and one macro for the high-level implementation pattern. What I'm thinking is that instead of a @generate_interface in front of all method definitions, you rather have a @generate_fallback_highlevel :CalculatorType and @generate_fallback_lowlevel :CalculatorType, which does introspection to see which methods the user has defined and defines the remaining ones (e.g. dispatch energy_forces_virial to energy_forces plus virial for @generate_fallback_highlevel and dispatch to calculate((Energy(), Forces(), Virial()) for @generate_fallback_lowlevel ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants