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

WIP: Prototypes for Calculators #86

Closed
wants to merge 3 commits into from
Closed

WIP: Prototypes for Calculators #86

wants to merge 3 commits into from

Conversation

cortner
Copy link
Member

@cortner cortner commented Sep 28, 2023

This PR goes with #84 - second attempt - for the time being we propose the following:

  • define minimal prototypes as in the first two commits.
  • document how they are intended to be used. (to be added to the docs) @tjjarvinen will do this and add
  • possibly provide tests that check for correctness of output, not clear this is needed for a first PR but could be nice to have.

Comments:

  • For now we are leaving out the general calculate prototype and will add it only if there is sufficient agreement on that.
  • We are not providing fallbacks since different use-cases lead to different fall-backs. This is a bit awkward and @tjjarvinen will instead look at other ways to provide this, possibly via macros.

Balancing the needs of classical (fast) FFs, MLFFs and electronic structure is a bit tricky which might lead to some strange looking choices initially. Please feel free to comment.

@rkurchin
Copy link
Collaborator

rkurchin commented Oct 2, 2023

As I remarked at #84 (comment), I like this general idea – however, I do wonder if the functionality is distinct enough from AtomsBase, which so far has focused pretty exclusively on specifying structures, that it should actually be a separate package.

@cortner
Copy link
Member Author

cortner commented Oct 2, 2023

I'd be happy with that too, but then that's another package to maintain and keep compatible with AtomsBase. My suggestion is to keep it here for now but if it grows too much or if it becomes invonvenient in any way to have the two linked in a single package then this can just be split off into a new package.

I want to make progress towards some of the potential demos we discussed.

@rkurchin
Copy link
Collaborator

rkurchin commented Oct 2, 2023

Yeah that's fair enough – I suppose the flexibility of Julian interfaces means that people could certainly implement only part of the interface in their package if it's truly not applicable for them.

@cortner
Copy link
Member Author

cortner commented Oct 2, 2023

I think the main debate is what the recommended approach should be

@mfherbst
Copy link
Member

mfherbst commented Oct 3, 2023

I'd be happy with that too, but then that's another package to maintain and keep compatible with AtomsBase. My suggestion is to keep it here for now but if it grows too much or if it becomes invonvenient in any way to have the two linked in a single package then this can just be split off into a new package.

Also linked to my comment in #84, but I agree that it's fine to have the stubs here at first and only factor out later if that's needed.

@cortner
Copy link
Member Author

cortner commented Oct 3, 2023

so the only question for now then is whether the experimental interface is fine as is or whether it needs more discussion.

@tjjarvinen
Copy link
Collaborator

Here is first draft.

There is documentation file calculations-interface.md that has details written out.

But basically you only need to define potential-energy, forces or forces! and virial to get the whole interface. You can customize for energy_forces_viral on top of that too.

The main point that in my opinion is important that we supply test functions to test the calculators, which I did.

There is still stuff that is missing. But you should already get the idea hot it works. Here is a small script to demonstrate

using AtomsBase
using Unitful
using UnitfulAtomic

struct MyType
end

struct MyOtherType
end

##

function AtomsBase.potential_energy(system, calculator::MyType; kwargs...)
    # we can ignore kwargs... or use them to tune the calculation
    # or give extra information like pairlist

    # add your own definition here
    return 0.0u"eV"
end

function AtomsBase.virial(system, calculator::MyType; kwargs...)
    # we can ignore kwargs... or use them to tune the calculation
    # or give extra information like pairlist

    # add your own definition here
    return zeros(3,3) * u"eV*Å"
end


AtomsBase.@generate_complement function AtomsBase.forces(system, calculator::Main.MyType; kwargs...)
    # we can ignore kwargs... or use them to tune the calculation
    # or give extra information like pairlist

    # add your own definition
    return zeros(AtomsBase.default_force_eltype, length(system)) 
end

AtomsBase.@generate_complement function AtomsBase.forces!(f::AbstractVector, system, calculator::Main.MyOtherType; kwargs...)
    @assert length(f) == length(system)
    # we can ignore kwargs... or use them to tune the calculation
    # or give extra information like pairlist

    # add your own definition
    for i in eachindex(f)
        f[i] = zero(AtomsBase.default_force_eltype)
    end

    return f
end


##


hydrogen = isolated_system([
    :H => [0, 0, 1.]u"bohr",
    :H => [0, 0, 3.]u"bohr"
])

AtomsBase.test_potential_energy(hydrogen, MyType())
AtomsBase.test_forces(hydrogen, MyType())
AtomsBase.test_virial(hydrogen, MyType())

AtomsBase.test_forces(hydrogen, MyOtherType())

##
AtomsBase.test_potential_energy(hydrogen, MyOtherType())  # this will fail
AtomsBase.test_virial(hydrogen, MyOtherType())  # this will fail

##

AtomsBase.potential_energy(hydrogen, MyType())
AtomsBase.forces(hydrogen, MyType())
AtomsBase.virial(hydrogen, MyType())
AtomsBase.energy_forces(hydrogen, MyType())
AtomsBase.energy_forces_virial(hydrogen, MyType())

@cortner
Copy link
Member Author

cortner commented Oct 30, 2023

Ready to close this?

@tjjarvinen
Copy link
Collaborator

From my part yes

@cortner
Copy link
Member Author

cortner commented Oct 30, 2023

Closing since this has been moved to AtomsCalculators.jl

@cortner cortner closed this Oct 30, 2023
@mfherbst mfherbst deleted the calculators branch October 23, 2024 17:51
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

Successfully merging this pull request may close these issues.

4 participants