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

Store additional properties in ASE calculator #658

Merged
merged 1 commit into from
Jul 22, 2024
Merged

Store additional properties in ASE calculator #658

merged 1 commit into from
Jul 22, 2024

Conversation

frostedoyster
Copy link
Contributor

@frostedoyster frostedoyster commented Jun 18, 2024

These changes allow for a user to calculate and store non-standard properties in the ASE calculator. For relatively experienced users, this is also a fix for #655 (one can set "energy", "forces", "stress" as properties_to_store, these will be calculated at each step, and then the inherited get_property function will not re-trigger a calculation but just read from the calc.results dict).

This PR now only contains code to compute energy/forces/stress when one of the three has been requested.

Contributor (creator of pull-request) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?
  • Issue referenced (for PRs that solve an issue)?

Reviewer checklist

  • CHANGELOG updated with public API or any other important changes?

📚 Download documentation preview for this pull-request

@Luthaf
Copy link
Member

Luthaf commented Jun 18, 2024

Why did you go with this approach instead of automatically storing energy/energies when computing the forces and stress? The fact that one has to explicitly set these is not ideal, and I feel like a solution to #655 that works without explicit user action would be nicer.

@frostedoyster frostedoyster force-pushed the ase-store branch 2 times, most recently from 1797ad8 to 42a8abd Compare June 21, 2024 08:31
@frostedoyster
Copy link
Contributor Author

This was not intended as a fix for #655 (although it is if one knows the internals). It's just to propagate potentially more outputs to the user

@Luthaf
Copy link
Member

Luthaf commented Jun 21, 2024

Ok, the initially intended API for doing outputs outside of ASE specification was the run_model function. Why is it not suitable in your case?

@frostedoyster
Copy link
Contributor Author

It looks to me like it recomputes everything...
IMO the only way not to recompute if one wants additional properties is to save stuff to the calculator.results dict, because that is what's checked to avoid recomputations (if the Atoms object hasn't changed)

@Luthaf
Copy link
Member

Luthaf commented Jun 21, 2024

It looks to me like it recomputes everything...

No, because this function is not integrated with the rest of ASE interface (get_property & friends). This should be used like this:

calculator = MetatensorCalculator(...)
outputs = {...}

calculator.run_model(atoms, outputs)

Since the user is explicitly calling this function themself, it will only recompute if the users call it again manually.


In general there are two separate issues IMO: #655 is about storing multiple that ASE knows about (storing energy when computing forces, etc.); while run_model is about computing properties ASE does not know about (i.e. mtt::last_layer_features, nmr_shielding, etc.).

We need a fix for #655, but I don't think that properties_to_store is the best solution. I would rather do this automatically without any user intervention.

@frostedoyster
Copy link
Contributor Author

frostedoyster commented Jun 24, 2024

The fact that it is not integrated with the rest of ASE is why the recomputations are necessary. If I need a quantity at some step of MD, I have to recompute, while in principle the same quantity would have been available together with the energy computation at near-zero overhead

@bananenpampe
Copy link

Hello,

Yuxuan and I have ported a preliminary chemical shielding model to an ase-MetatensorCalculator.
This model returns atom centered non standard properties, namely the chemical shieldings.
I would feel like, that the most natural way to compute these properties woule be like this (ideally even without having to specify the domain name itself):

calc = MetatensorCalculator("model.pt")
frame.set_calculator(calc)
cs_iso = frame.get_properties(["mtt::cs_iso"])

However, currently it requires doing this, which I find not so intuitive:

output = calc.run_model(frame, outputs)
cs_iso = output['mtt::cs_iso'].block(0).values.detach().numpy()

I personally feel like the results should be stored in the results dict as numpy arrays, that way models can be used from users outside the metatensor universe.

@Luthaf
Copy link
Member

Luthaf commented Jul 8, 2024

This PR would remove the need to explicitly call run_model and instead put the data in a dict, but you would still have to provide the ModelOutput and you would get the results as TensorMap.

I personally feel like the results should be stored in the results dict as numpy arrays, that way models can be used from users outside the metatensor universe.

This is not possible for fully generic metatensor outputs, since they could have multiple keys/blocks, and then this function would not know what to do with them.

I agree that doing things like this would be nice, but it does not fit in the ASE calculator framework. We could however define a new API that could do what you are asking for standardized outputs, with something like this

calc = MetatensorCalculator("model.pt")
cs_iso = calc.get_metatensor_property(atoms, "scalar_chemical_shielding")

assert isinstance(cs_iso, np.ndarray)

This requires first defining what the standard outputs are and what get_metatensor_property should do for these outputs.
For example, what does this function do if there are multiple blocks in the output? What does it do if there are missing samples? How does it differentiate per atom or per structure outputs?

I'll open a separate issue to discuss this further, this PR is mainly aimed at things that should be computed together with the energy/forces.

@Luthaf Luthaf merged commit c19ef37 into master Jul 22, 2024
23 checks passed
@Luthaf Luthaf deleted the ase-store branch July 22, 2024 09:02
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.

3 participants