AtomsCalculators.jl compatibility

[jgreener64]

Maybe @tjjarvinen could check this over quickly to see if it looks okay.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2eb4712) 72.39% compared to head (b51b441) 72.53%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
+ Coverage   72.39%   72.53%   +0.13%     
==========================================
  Files          35       35              
  Lines        5231     5246      +15     
==========================================
+ Hits         3787     3805      +18     
+ Misses       1444     1441       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tjjarvinen]

Looks mostly good, but I have couple of comments.

Force type promote

You should also extend promote_force_type function. This function is used to allocate force data for non allocating force call. Something like this should do

function AtomsCalculators.promote_force_type(::Any, mc::MollyCalculator)
    return SVector(1., 1., 1.) * mc.force_units |> typeof
end

So, check that AtomsCalculators.promote_force_type and AtomsCalculators.zero_forces return correct force type and force data structure. The documentation for these is bit lagging, so I need to add more on them...

Number of threads

MollyCalculator defines number of threads used. Would it be better to move this to a kword?

The second part on this, this is something I was going to ask you in a separate issue than this PR, but here we go... The use of name n_threads with explanation "the number of threads to use when running the calculator" - this is actually miss leading.

What actually happens is that Julia thread count is fixed on startup, you cannot change it later. What happens in practice, is that you spawn certain number of tasks and those task get distributed on the active thread pool, which by default is all threads. So, to be accurate, it would be better to have name n_tasks with a description "number of tasks spawn to perform the calculation".

For optimal use n_tasks should be at least the number of threads. In many cases you will also get extra performance by over subscribing, but allocations affect this and for force call it might not happen.

So, if you combine these, you could implement forces like this

AtomsCalculators.@generate_interface function AtomsCalculators.forces(
        abstract_sys,
        calc::MollyCalculator;
        neighbors=nothing,
        n_tasks=Threads.nthreads(),
        kwargs...,
    )
    sys_nointers = System(abstract_sys, calc.energy_units, calc.force_units)
    sys = System(
        sys_nointers;
        pairwise_inters=calc.pairwise_inters,
        specific_inter_lists=calc.specific_inter_lists,
        general_inters=calc.general_inters,
        neighbor_finder=calc.neighbor_finder,
        k=calc.k,
    )
    nbs = isnothing(neighbors) ? find_neighbors(sys) : neighbors
    return forces(sys, nbs; n_threads=t_tasks)
end

AtomsCalculators support in Molly calculations

Can Molly use AtomsCalculators to calculate general forces?

[jgreener64]

Thanks for that. I have defined promote_force_type and tested the zero forces. I also moved n_threads to a keyword argument of the calculator functions.

Your point about the threads seems sensible, I will put it on the todo list though as it will need a (breaking) change to the whole package to implement consistently throughout.

Can Molly use AtomsCalculators to calculate general forces?

Possibly, did you have a particular interface in mind? I guess calculators could be given in the general interaction tuples.

[tjjarvinen]

I guess calculators could be given in the general interaction tuples.

Yes, this is what I had in my mind. It would make easy to add custom calculators for Molly.

For a longer term option, integrating AtomsCalculators more inside Molly is probably the way to go. Like use making force calls unified etc. But, for now adding support for general interactions is enough.

[jgreener64]

I'll put using calculators for general interactions on the todo list, I can't estimate when I'll have time though. Feel free to have a go if you have time.

[CedricTravelletti]

I'll put using calculators for general interactions on the todo list, I can't estimate when I'll have time though. Feel free to have a go if you have time.

Is there any update on this?
This could be nice to have to demonstrate the modularity of the ecosystem, and is just one step away since DFTK now provides an AtomsCalculators interface.

[jgreener64]

I'll try to start it this week.

[jgreener64]

I switched general interactions to use the AtomsCalculators.jl interface: 66f1723. In the end it was a fairly small change.