Adding a conversion function for other packages #139
Conversation
|
The documentation for this PR is (or will soon be) available on readthedocs: https://rascaline--139.org.readthedocs.build/en/139/ |
|
Here is a pre-built version of the code in this pull request: wheels.zip, you can install it locally by unzipping |
There was a problem hiding this comment.
This is very useful, thanks a lot!
However I'm not sure if putting this function inside rascaline is the best thing to do from a marketing/future of the library point of view. I think this would be better as a semi-external tool (maybe a small website?) in which users can copy/paste hypers, forcing them to update there code. Right now I could see people keep the other packages syntax in their code, and use these function to dynamically translate to rascaline, while it would be better if they directly learn to work with rascaline hypers.
Alternatively, have this function print "your rascaline hypers are ..." as a string would work for me.
| ) | ||
|
|
||
| not_supported = [ | ||
| "coefficient_subselection", |
There was a problem hiding this comment.
coefficient_subselection could point people to the selected_properties argument of the compute function
| "global_species", | ||
| "expansion_by_species_method", | ||
| "soap_type", | ||
| "compute_gradients", |
There was a problem hiding this comment.
compute_gradients could point people to the gradients argument of the compute function
| not_supported = [ | ||
| "coefficient_subselection", | ||
| "covariant_lambda", | ||
| "gaussian_sigma_type", |
There was a problem hiding this comment.
gaussian_sigma_type="Constant" (which is the only one implemented by librascal) is also what rascaline is doing by default
| "optimization_args", | ||
| "optimization", |
There was a problem hiding this comment.
Some of these could be translated to "Gto" radial basis options in rascaline
| if new_hypers["radial_basis"] != "Gto": | ||
| warnings.warn("WARNING: rascaline currently only supports a Gto basis.") | ||
|
|
||
| deprecated_params = ["average", "sparse", "dtype"] |
There was a problem hiding this comment.
average="outer" could point to equistore.operations.mean_over_samples, and average="off" is the default
| "crossover", | ||
| "average", | ||
| "sparse", | ||
| "dtype", |
There was a problem hiding this comment.
there is also weighting which can be translated to either radial_scaling (weighting/function/pow in dscribe) or central_atom_weight (weighting/w0 in dscribe)
|
|
||
| not_supported = [ | ||
| "periodic", | ||
| "crossover", |
There was a problem hiding this comment.
crossover=True is what rascaline does, crossover=False could be achieved once #134 is merged with a selected_keys parameter to the compute function
| with self.assertRaises(ValueError): | ||
| convert_old_hyperparameter_names({}, mode="BadMode") |
There was a problem hiding this comment.
please also check the error message when checking that exceptions are raised, otherwise it is really easy to get the tests to pass with a different error than the expected one beening raised.
| with self.assertRaises(ValueError): | |
| convert_old_hyperparameter_names({}, mode="BadMode") | |
| with self.assertRaises(ValueError) as cm: | |
| convert_old_hyperparameter_names({}, mode="BadMode") | |
| self.assertEqual(str(cm.exception), "some error message") |
| convert_old_hyperparameter_names({"bad_param": 0}, mode="dscribe") | ||
|
|
||
| def test_not_gto(self): | ||
| with warnings.catch_warnings(record=True) as w: |
There was a problem hiding this comment.
that works, but I've been using self.assertWarns everywhere else, so it might be better to use it here as well for consistency
|
linter is not happy, you can run |
Co-authored-by: Guillaume Fraux <[email protected]>
|
I think this is worth having and tracked in #264, but in the mean I'll close this PR for inactivity. We can re-open something later! |
Just because I was writing it for myself and figured others would fine it useful. Added all of the necessary tests and included a shout-out in docs where I thought it was appropriate.