Document the purpose and use of clean_value#2295
Merged
sphuber merged 2 commits intoDec 7, 2018
Merged
Conversation
sphuber
reviewed
Dec 5, 2018
Contributor
There was a problem hiding this comment.
idempotent is not the correct term here. An action is said to be idempotent if it can be invoked any number of times and the aggregate result is always the same, regardless of the number of times it was called. What I think we want to say here is that the clean_value will transform the value such, that a serialization/deserialization round trip through the database will return the same value.
Also we should add after the mention that Iterables are converted to lists, that Mappings will be converted into plain dictionaries.
Finally, there is a small typo defualt -> default
Contributor
Author
There was a problem hiding this comment.
Thanks! Changes made.
ConradJohnston
force-pushed
the
fix_2038_clean_value
branch
from
57564cc to
00c3c35
Compare
- Update the docs to explain the use of clean_value and its behaviour. - Update clean_value doc string to better explain its purpose. - Change collection imports from, for example, "import collections.Iterable" to "from collections import Iterable" to comply wth a forthcoming deprecation that will break these imports in Python 3.8.
ConradJohnston
force-pushed
the
fix_2038_clean_value
branch
from
00c3c35 to
7929d23
Compare
|
|
||
| Node methods | ||
| ****************** | ||
| - :py:meth:`~aiida.orm.implementation.general.node.clean_value` takes a value and returns an object which can be serialized for storage in the database. Such an object must be able to be subsequently deserialized without changing value. If a simple datatype is passed (integer, float, etc.), a check is performed to see if it has a value of ``nan`` or ``inf``, as these cannot be stored. Otherwise, if a list, tuple, dictionary, etc., is passed, this check is performed for each value it contains. This is done recursively, automatically handling the case of nested objects. It is important to note that iterable type objects are converted to lists during this process, and mappings, such as dictionaries, are converted to normal dictionaries. This cleaning process is used by default when setting node attributes via :py:meth:`~aiida.orm.implementation.general.node.AbstractNode._set_attr` and :py:meth:`~aiida.orm.implementation.general.node.AbstractNode._append_to_attr`, although it can be disabled by setting ``clean=False``. Values are also cleaned when setting extras on a stored node using :py:meth:`~aiida.orm.implementation.general.node.AbstractNode.set_extras` or :py:meth:`~aiida.orm.implementation.general.node.AbstractNode.reset_extras`, but this cannot be disabled. |
Member
There was a problem hiding this comment.
A couple of small notes here:
- I don't think
NaNandinfare already checked - this is part of SQLalchemy: storing ParameterData object with "nan" value of an attribute raises an exception #1498 (for me it's fine if you want to solve both issues in this PR but then you need to add the logic to check for these values). - maybe a short mention that at
.store()clean_value is not called, so if you useclean=Falseyou should be very sure that you are only using base types (e.g. not tuples but lists etc.) otherwise e.g. the hashing will be wrong, and depending on the DB backend you can either get an exception while storing (I guess in the case of numpy arrays), or the type can be converted silently (I guess in the case of tuples).
Contributor
Author
There was a problem hiding this comment.
Hi Giovanni, looking again at this, it's looks like clean_builtin() does this check:
https://github.com/aiidateam/aiida_core/blob/7929d2333c3811c9ef5c520c765f32eac71e3b0c/aiida/orm/implementation/general/node.py#L61
Member
giovannipizzi
approved these changes
Dec 7, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2038 and #1498
"from collections import Iterable" to comply with a forthcoming
deprecation that will break these imports in Python 3.8.