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 Energy, Forces, Virials #85

Closed
wants to merge 1 commit into from
Closed

Conversation

cortner
Copy link
Member

@cortner cortner commented Sep 27, 2023

This PR goes with #84 - For the time being I propose to do no more than

  • define minimal prototypes as in the first commit.
  • document how they are intended to be used. (to be added to the docs)
  • possibly provide tests that check for correctness of output, not clear this is needed for a first PR but could be nice to have.
  • some default fallbacks; cf Teemu's post below

For now I am leaving out the general calculate prototype and will add it only if there is at least partial agreement on that.

@cortner
Copy link
Member Author

cortner commented Sep 27, 2023

@tjjarvinen -- can you please add a draft documentation page that explains the interface. I think we are more or less on the same page.

@cortner cortner removed the request for review from jgreener64 September 27, 2023 04:03
@tjjarvinen
Copy link
Collaborator

I would actually add here that maybe the default should also include:

function forces!(f, data, calc; kwargs...)
    f .= forces(data, cal; kwargs...)
    return f
end

function energy_forces_virial(data, calc; kwargs...)
    e = energydata, calc; kwargs...)
    f = forces(data, calc; kwargs...)
    v = forces(data, calc; kwargs...)
    return (; :energy=>e, :forces=>f, :virial=>v)
end

etc.

Also, I would say that energy_forces function should be part of the interface too.

@cortner
Copy link
Member Author

cortner commented Sep 27, 2023

Yes, something like that should be added, I forgot to add this to the list at the top.

The reason I didn't yet add it to the code, is I'm not sure which should the default be? Don't we want the codes to implement forces! and then provide a wrapper

forces(sys, calc) = forces!(alloc(...), sys, calc)

@cortner
Copy link
Member Author

cortner commented Sep 27, 2023

regarding energy_forces I'm not sure, couldn't one simply do

E, F, _ = energy_forces_virial

but I dont have a strong view on this.

What about forces_virial and forces_virial!? There are so many variants.

Somehow I'd prefer to focus on a minimal set and the two

potential_energy 
energy_forces_virial!

seems to be the two natural ones to implement.

@tjjarvinen
Copy link
Collaborator

tjjarvinen commented Sep 27, 2023

No. Not all methods can calculate virial and you want those to throw error. This is the case e.g. in isolated molecules, where virial is meaningless.

@cortner
Copy link
Member Author

cortner commented Sep 27, 2023

One can equally argue that the virial is simply zero in that case since the energy doesn't change when you change cell shape.

@tjjarvinen
Copy link
Collaborator

If you do constant volume simulation, you don't want to spend time to calculate virials. So, calling a method that calculates virials is a waste.

For me energy_forces and energy_forces_virial are the ones we should have, others are special cases.

@cortner
Copy link
Member Author

cortner commented Sep 27, 2023

If you do constant volume simulation, you don't want to spend time to calculate virials

It has essentially zero extra cost if you already compute the forces.

But ok, let's assume this. then why energy_forces and not energy_forces!? For electronic structure or large MLIPs models it doesn't matter but e.g. for a fast empirical potential the allocation might matter.

And why should potential_energy not have a separate implemenation? In many codes, evaluating only energy could be much faster than energy and forces.

@tjjarvinen
Copy link
Collaborator

I meant for the combinations only energy_forces and energy_forces_virial and the non-allocating ones for them.

It has essentially zero extra cost if you already compute the forces.

For our ACE code it is not essentially free. It takes a considerable amount of time to gather the final value.

@cortner
Copy link
Member Author

cortner commented Sep 27, 2023

I don’t think you are right - let’s benchmark this and stop arguing here.

@cortner
Copy link
Member Author

cortner commented Sep 27, 2023

I think this debate had become unproductive. I’m closing the PR and will open a new one with a more succinct proposal.

@cortner cortner closed this Sep 27, 2023
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