"node_energy" in implemented properties make ase singlepoint calculator angry!

[sandipde]

mace/mace/calculators/mace.py

Line 78 in 5debea4

"node_energy",

Assertion Fails in https://gitlab.com/ase/ase/-/blob/master/ase/calculators/singlepoint.py?ref_type=heads#L25
as "node_energy" is not one of the defined properties in https://gitlab.com/ase/ase/-/blob/master/ase/calculators/calculator.py?ref_type=heads#L125

we can still keep the node energy and all the other stuff in the results but probably it's best to remove non-standard items from implemented_properties

or we ask ase to remove the assertation. just wanted to bring up this issue that I just faced!

[davkovacs]

Interesting, I have never seen this. @gabor1 what do you think? Shall we remove it form there, and just write to results dict? I am not sure who is familiar with ase conventions, maybe @bernstei , do you have an opinion on this?

[bernstei]

I'm not sure what the state of the art is. Certainly we've had issues with non-standard properties in the results dict for things like converting to a SinglePointCalculator. I think GAP uses an extra_results dict maybe? I'll ask for opinions in the ASE slack channel.

[jameskermode]

if node_energy is just the per-atom energy, then using the ASE standard property name energies would solve the problem, as that’s in the list of expected properties. Otherwise non standard results should indeed be stored elsewhere.

[Andrew-S-Rosen]

That is strange. I have used custom properties with the FileIOCalculator and GenericFileIOCalculator. An example of the former is here. Could you try defining them as class variables before the init? If that doesn't help, maybe it's a limitation of Calculator?

[bernstei]

SinglePointCalculator (https://gitlab.com/ase/ase/-/blob/master/ase/calculators/singlepoint.py?ref_type=heads) definitely uses lists of possible properties from ase-wide modules like ase.calculators.

[sandipde]

@bernstei if you are referring to line 24 and 26 in the link you provided, you can test, it does not work

            assert property in all_properties, property

an easy test to confirm this

allprop=['a','b','c','d']
myprop=['d','e']
for p in myprop:
    assert p in allprop,p

---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[3], line 2
      1 for p in myprop:
----> 2     assert p in allprop,p

AssertionError: e

it needs to be modified as
assert p in [*allprop,p]

at least that's how it is in python3.10.12

[bernstei]

That is the expected behavior: 'e' isn't in allprop, so it errors out, telling you that it's e that it didn't find. Your syntax is meaningless - it would never give an assertion error, because p will always be in the list it's comparing to.

With assert, the value after the comma is the reason the assertion error will print, not part of the in comparison.

[sandipde]

@bernstei thats what my first point when I raised this issue. As node_energy is not defined in all supported property list in ase, this causes the issue.

[bernstei]

@bernstei thats what my first point when I raised this issue. As node_energy is not defined in all supported property list in ase, this causes the issue.

Yes - that's why you can't stick random properties in the results dict. You can change this restriction, but the problem is that readers/writers (this came up in extxyz, hence my and @jameskermode's involvement) need to know what calculated properties are per-config, and go in Atoms.info and the extxyz "comment" line, and what are per-atom, and go in Atoms.arrays or columns of the per-atom rows. Without a standardized properties list that specifies with which ones are per-config and which are per-atom, there's no way to tell. That's why this restriction exists.

Your

it needs to be modified as
assert p in [*allprop,p]

is definitely not a fix - that test does nothing, as it always returns True. If we decide that's how it should operate, then just remove the assert. But it'll break things.

[sandipde]

I think we are on the same page @bernstei . My very first suggestion and motivation behind raising this issue was to avoid including random properties.

[wcwitt]

Is there a reason not to switch to energies?

[bernstei]

I think we are on the same page @bernstei . My very first suggestion and motivation behind raising this issue was to avoid including random properties.

OK - I guess I was just confused by your proposed change to the assert.

[sandipde]

I think we are on the same page @bernstei . My very first suggestion and motivation behind raising this issue was to avoid including random properties.

OK - I guess I was just confused by your proposed change to the assert.

No worries, I got the impression from your earlier comment that you did not see any problem with this issue.

SinglePointCalculator (https://gitlab.com/ase/ase/-/blob/master/ase/calculators/singlepoint.py?ref_type=heads) definitely uses lists of possible properties from ase-wide modules like ase.calculators.

I think then we are on the same page that we can rename this node_energy to energies as per ase convention.

[gabor1]

We used to make this decision based on the shape, ie if one dimension was Natoms then you assume it's an atomic property. This breaks down eg for stress like quantitaties and 3-atom structures, but maybe we could revisit this, and try to recognise per-config tensors by their standard names (stress, virial etc) and apply the heuristic to the rest.