-
Notifications
You must be signed in to change notification settings - Fork 2
Adoption to new easyscience (fitting, parameter unique name #173
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
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.
This pull request does not contain a valid label. Please add one of the following labels: ['chore', 'fix', 'bugfix', 'bug', 'enhancement', 'feature', 'dependencies', 'documentation']
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 seems to look OK, I had several general questions, but nothing prohibiting from the merge.
super().__init__(name, interface, *models, **kwargs) | ||
self.interface = interface | ||
|
||
# Needed by the as_dict functionality | ||
self.populate_if_none = False |
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.
there's no description of this boolean. Why is it needed in as_dict?
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 code comment:
# Needed to ensure an empty list is created when saving and instatiating the object as_dict -> from_dict
# Else collisions might occur in global_object.map
'thickness': self.thickness, | ||
'discretisation_elements': self._discretisation_elements, | ||
'thickness': float(self.thickness), | ||
'discretisation_elements': int(self._discretisation_elements), |
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.
why are those explicit coversions needed?
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.
PR response
These are properties that else appears weird in the dictionary
No intended changes to functionality only adoption to the new easyscience lib.
Main changes are
raw_value
->value
uid
->unique_name
as_dict
andfrom_dict
populate_if_none
to prevent the creation of internal variables when creating an object usingfrom_dict
unique_names
are determined for internal variables.When testing:
dict_round_trip
(as_dict
andfrom_dict
) testsas_dict
andfrom_dict
tests