CederGroupHub / chgnet

Pretrained universal neural network potential for charge-informed atomistic modeling https://chgnet.lbl.gov
https://doi.org/10.1038/s42256-023-00716-3
Other
226 stars 61 forks source link

ASE calculate() method should have properties and system_changes arguments #34

Closed mhellstr closed 1 year ago

mhellstr commented 1 year ago

The CHGNetCalculator in model/dynamics.py is defined with

    def calculate(
        self,
        atoms: Atoms | None = None,
        desired_properties: list | None = None,
        changed_properties: list | None = None,
    ) -> None:

To be ASE compatible (ase/calculators/calculator.py) it would be great if it could support the properties and system_changes args.

def calculate(self, atoms=None, properties=['energy'],
                  system_changes=all_changes):
BowenD-UCB commented 1 year ago

desired_properties and changed_properties are fed to the ASE base calculator:

https://github.com/CederGroupHub/chgnet/blob/95976d69843b39ac28630616159c751fb931d3a3/chgnet/model/dynamics.py#L87-L108

But these two keys are redundant for CHGNet calculation because, in the current setup, CHGNet will always calculate the energy, forces, stress, and magmoms in the calculator.

Can you provide more information on how you want to use the properties and system_changes that the current CHGNet calculator cannot do?

mhellstr commented 1 year ago

The problem is my code assumes the arguments are called properties and system_changes. So it would be nice if it would be possible to just rename them to be consistent with the rest of ASE.

By the way, for most of the test cases I've played with CHGNet seems to work great!

BowenD-UCB commented 1 year ago

Thanks for this comment, it should be a valuable change. This was fixed in 628c62fe087907fc3e3d2a67d3642861706833a9.