search

lab-cosmo / librascal

A scalable and versatile library to generate representations for atomic-scale learning
https://lab-cosmo.github.io/librascal/
GNU Lesser General Public License v2.1
80 stars 20 forks source link

Misleading name and documentation for `Spherical(In|Co)variants.update_hyperparameters()` #352

Open max-veit opened 3 years ago

max-veit commented 3 years ago

Judging from the name and documentation of this function -- e.g. here: https://github.com/cosmo-epfl/librascal/blob/43a410c6d7cea3d17852e9c8c6fe332ec46f13df/bindings/rascal/representations/spherical_expansion.py#L236-L240 -- I would expect this function to also update the parameters of the underlying C++ object, so that further calls to the transform() method use the representation with the updated parameters. However, the code makes it clear that this is not the case, and that this is simply a parameter validation method for internal use only.

Fix: Rename the function and documentation to reflect this fact. Instead of allowing (or promising to allow) updating of hypers on the fly, just encourage the user to make a new representation.

agoscinski commented 3 years ago

Suggestion for rename of the function: _shallow_update(self, **hypers) The private makes the user aware that it should be used with care and the hypers can be induced from the input arguments.

agoscinski commented 2 years ago

We should use the hacky solution for a new update_hyperparameters() function

max-veit commented 2 years ago

Sorry, what hacky solution?

agoscinski commented 2 years ago

Ah sorry, I think I commented before finishing the whole comment. @cbenmahm had a solution, by basically creating a new Spherical(In|Co)variants object. "hacky" because we don't really update parameters, but ditch the old object. Let me finish #394 and I go back on this issue