Luthaf / rascaline

Computing representations for atomistic machine learning
https://luthaf.fr/rascaline/
BSD 3-Clause "New" or "Revised" License
44 stars 13 forks source link

Naming of gradients #161

Closed agoscinski closed 4 months ago

agoscinski commented 1 year ago

Hello, I wonder if we should switch the way we name gradients, because at the moment the metadata for the components is not so descriptive. For the positions gradients it is obvious that the three components correspond to x y z in that order, but for cell gradients I think it would be useful to be more descriptive. Here the stress could be stored in Voigt notation or as a matrix. Sure one can always check the documentation for that or do a educative guess, but since we already have the metadata feature, then we can make use out of it and make it more user friendly. I use the forces for examples, but I am thinking that this change is more useful for the stress.

One could do reshape the components to have a name for each one

# Gradient of TensorBlock would look like this
data=array of shape (n_samples, 1, 1, 1, n_properties)
components=[
                  Labels(["x"], np.array([[0]], dtype=np.int32)),
                  Labels(["y"], np.array([[0]], dtype=np.int32)),
                  Labels(["z"], np.array([[0]], dtype=np.int32))
]

but it probably simplifies the usability if we keep the forces in one dimension, therefore I thought about just changing the name

# Gradient of TensorBlock would look like this
    components=[Labels(["direction (x y z)"], np.array([[0], [1], [2]], dtype=np.int32))],

What do you think?

EDIT: Okay just realized that "direction (x y z)" is not a valid Label name. One could also use "direction_x_y_z", but I am not sure if there are better solutions for this.

Luthaf commented 1 year ago

One could do reshape the components to have a name for each one

that does not work, because the three components of the gradients can not be separated: one need to handle x, y and z together, and not as separate entities.

therefore I thought about just changing the name

I'm happy to find a better name for these components! The current limitation is that names should be identifiers, because we are using numpy structured arrays to access the Labels. But once we change this (Point 2 in https://github.com/lab-cosmo/equistore/issues/133), we could use names with spaces inside.

Here the stress could be stored in Voigt notation or as a matrix.

We can not use the Voigt notation, because we don't have a guarantee that the stress is symmetric when handling per-atom stress (only the per-structure stress will be symmetric). But here as well we could fine better names for the components.

Luthaf commented 4 months ago

This has been decided and we now follow the convention in https://docs.metatensor.org/latest/atomistic/outputs.html