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

Calculator State #961

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Conversation

CedricTravelletti
Copy link
Contributor

This introduces state updating in the calculator.

See this PR in GeometryOptimization.jl for a detailed discussion of why this is needed.

@mfherbst
Copy link
Member

Lets wait for JuliaMolSim/AtomsCalculators.jl#11 and see where this settles.

BTW: It's a bit weird your PRs always show these millions of commits if you only add a few things. Any idea what that could be ?

@CedricTravelletti
Copy link
Contributor Author

Yes, I know why there are so many commits: What is done in this PR (and in the other ones I submitted) is part of a cross-package effort.

What I mean is that the PR in DFTK.jl is just a side effect of what we want to do in GeometryOptimization.jl, but changes will also then ripple on to AtomsCalculators.jl (and in the end we want to use everything in InverseDesign.jl).

So things evolve in parallel and create a back and forth commit cascade before the final PR is settled.

@mfherbst
Copy link
Member

mfherbst commented Jul 28, 2024

Just pushed an update to get this compatible to AtomsCalculator 0.2. There is a key issue remaining: We are not able to benefit from reusing state in calls like energy_forces or calculate((Energy(), Forces()). This is due to some missing features in AtomsCalculators, which I don't have time to fix right now.

In short:

  • The implementation of calculate(::Tuple ) does not transfer the state from one tuple element to the next
  • energy_forces dispatches not to the low-level interface, hence no state-sharing is possible.

I would still suggest we merge as is, just to have the update to 0.2 done and then proceed to fix the issues in AtomsCalculators.

See JuliaMolSim/AtomsCalculators.jl#27

@mfherbst mfherbst merged commit 929519e into JuliaMolSim:master Jul 29, 2024
6 of 8 checks passed
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.

2 participants