Skip to content

Conversation

@ppegolo
Copy link
Contributor

@ppegolo ppegolo commented Sep 10, 2025

Contributor (creator of pull-request) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?

Reviewer checklist

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

📚 Download documentation for this pull-request

Copy link
Member

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks fine overall, it would need some tests and documentation (maybe even worth a separate tutorial!)

@ppegolo ppegolo marked this pull request as ready for review October 9, 2025 12:07
@ppegolo ppegolo requested review from Luthaf and jwa7 October 9, 2025 12:55
Copy link
Member

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, thanks a lot!

Do you also want to add a tutorial here explaining why one would want to use this (I guess computing phonons?)

:py:meth:`ase.Atoms.get_forces`, …);
- arbitrary outputs can be computed for any :py:class:`ase.Atoms` using
:py:meth:`MetatomicCalculator.run_model`;
- for non-equivariant architectures like PET, rotatonally-averaged energies, forces,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a link to PET in metatrain, and maybe also link to another non-equivariant architecture?

Comment on lines +888 to +892
self,
base_calculator: MetatomicCalculator,
l_max: int = 3,
batch_size: Optional[int] = None,
include_inversion: bool = True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self,
base_calculator: MetatomicCalculator,
l_max: int = 3,
batch_size: Optional[int] = None,
include_inversion: bool = True,
self,
base_calculator: MetatomicCalculator,
*,
l_max: int = 3,
batch_size: Optional[int] = None,
include_inversion: bool = True,

This forces people to pass all args as kwargs instead of allowing for positional arguments

l_max: int = 3,
batch_size: Optional[int] = None,
include_inversion: bool = True,
apply_group_symmetry: bool = False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is about crystal symmetry group, correct? Or can it also handle molecule point group?

We may want to rename this because I initially read "group" as the SO(3) group and did not understand the point of this ^_^

"Out of memory error encountered during rotational averaging. "
"Please reduce the batch size or use lower rotational "
"averaging parameters. This can be done by setting the "
"`batch_size`, `lebedev_order`, and `n_inplane_rotations` "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n_inplane_rotations does not exists anymore

Comment on lines +880 to +881
:param return_samples: if ``True``, the results of the base calculator on each
rotated system will be returned. Most useful for debugging.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to keep this around? Which use case do you foresee for this?

if "energy" in results:
E = np.asarray(results["energy"], dtype=float)
out["energy"] = _wmean(E)
out["energy_rot_std"] = _wstd(E)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These does not seem to be documented, and I'm mildly against adding custom output to a standard interface. What's the use case for these?

IMO if we keep them around they should at least be behind an option store_rotational_std=True (or another similar name).

# Forces (B,N,3) from rotated structures: back-rotate with F' R
if "forces" in results:
F = np.asarray(results["forces"], dtype=float) # (B,N,3)
F_back = np.einsum("bnj,bjk->bnk", F, R, optimize=True) # F' R
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

einsum (here and for stress) tend to be very slow, was the speed fine for you here? Maybe the model is slower anyway.

- arbitrary outputs can be computed for any :py:class:`ase.Atoms` using
:py:meth:`MetatomicCalculator.run_model`;
- for non-equivariant architectures like PET, rotatonally-averaged energies, forces,
and stresses can be computed using :py:class:`SymmetrizedCalculator`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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