-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adapt cpp representation input paramaters to the ones on the python side #376
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good; my only worry is the duplication of information between the C++ and Python representation info. If someone were to decide to edit this file (JSON is human-readable, after all), this might result in a Python object with inconsistent parameters in C++, which would be bad. One way to prevent this would be to have a consistency check (i.e. that the C++ parameters match what is constructed using the Python parameters) - although ideally, we wouldn't need to duplicate this information at all, probably by just storing the C++ representation and having the Python load/save functions work with this format.
bindings/rascal/models/kernels.py
Outdated
if "cpp_kernel" in data.keys(): | ||
self._kernel = self._kernel.from_dict(data["cpp_kernel"]) | ||
else: | ||
print( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider replacing with LOGGER.warn()
-- NB needs the following near the imports:
import logging
LOGGER = logging.getLogger(__name__)
(see e.g. bindings/rascal/utils/filter.py
for an example of how this can be done)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added LOGGER
@@ -5236,4 +5332,4 @@ | |||
}, | |||
"module_name": "rascal.models.krr", | |||
"version": "0.1" | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
(git doesn't like it when files end without newlines)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
@@ -5119,7 +5119,52 @@ | |||
"init_params": { | |||
"representation": { | |||
"class_name": "SphericalInvariants", | |||
"data": {}, | |||
"data": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplicates all the information already contained below (in "init_params"
), which is not ideal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue has been solved by adapting cpp input parameters to python input parameters
* cpp representation and kernel information is now included in rascal's model json file. * weights are now always flattened to one dimension for consistency in the models format.
…; make gaussian_sigma_type Constant default parameter
… cpp side for global_species coefficient_subselection optimization
50c0ccf
to
fec2c0c
Compare
see updates PR summary at top |
Changes:- cpp representation and kernel information is now included in rascal's modeljson file.
- weights are now always flattened to one dimension for consistency in themodels format.
- update rascal models json in testsThis allows so simply adapt your model so it is usable with the lammps rascal interfaceI am aware that #305 discusses more general changes in the format, but this PR only adds cpp information to the model json, so I think we can have it as a separate PR. Also I would like that people are not required to just use a branch for 3 lines of code. I think PR #305 will still take some time until it is merged.Tests intests/python/md_calculator_test.py
should cover the changes! This PR does not break backwards compatibility to old rascal gap model jsons !
I decided to spent the time to adapt the c++ interface to the python interface, because it is too easy that undetected inconsistencies between cpp and pythoh hypers can happen. We could only do a consistency check on the python side, but from the c++ we cannot really do it (without putting similar effort as adapting hypers on both sides)and I see there a lot of annoying-to-debug problems arising, especially with default arguments on both sides.
Since even simple errors in the json file (e.g. typo in a json key as "cutof") are almost untrackable without printing the hell out of classes in the current state (basically the error messages says "there is somewhere something wrong with the json), I updated all error messages for reading the json file. This is also important for a user using the c++ interface to get meaningful errors. This makes our life dramatically easier to debug the tests. For this I just add the FILE and LINE information in each error similar is it is done in LAMMPS. I have a macro for FILENAME so it takes only the relative rascal path and not the absolute path (e.g. note "/ssd/local/code/librascal/src/rascal/structure_manager/structure_manager.hh" but only "src/rascal/structure_manager/structure_manager.hh")
So to sum up the two main changes now:
Additonal small changes:
gaussian_sigma_type
to"Constant"
which is the only possible value it can take.Supplementary
For transforming the ubjson reference data (in the cpp tests) I used the following script to not touch the reference results
same for the
kernel_reference.ubjson
for hypers indata['rep_info']['spherical_invariants'][i][j]['hypers_rep']