Fix potential inefficiency in aiida.tools.data.cif converters
#3098
Conversation
This file contains 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
Recently, the `BackendNode` interface and implementation was changed significantly with respect to attributes and extras. As a result, the values on unstored nodes are not cleaned *until* the node is stored. That means that unstored nodes can contain attributes and extras with uncleaned values. For example an unstored `Dict` node can contained stored `Float` nodes. This change becomes problematic in the implementation of `_get_aiida_structure_pymatgen_inline` and `_get_aiida_structure_ase_inline` of `aiida.tools.data.cif`. The `parameters` keyword argument can be an unstored `Dict` node, if the functions are run with `store_provenance=False`. Since it is unstored, it can contain *stored* nodes for the values within them. For example, the `site_tolerance` can be a stored `Float`. Since they are overloaded native float objects, they can be passed to pymatgens `CifParser` without it complaining. However, now whenever that class references the value, since it is a stored node, the `ModelWrapper` will cause the model instance to be refreshed from the database, which becomes prohibitively expensive. Simply calling `clean_value` on the `parameters` beforehand, will cause these nodes to be dereferenced into their base values, which solves the issue.
giovannipizzi
approved these changes
Jun 30, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #3097
Recently, the
BackendNodeinterface and implementation was changedsignificantly with respect to attributes and extras. As a result, the
values on unstored nodes are not cleaned until the node is stored.
That means that unstored nodes can contain attributes and extras with
uncleaned values. For example an unstored
Dictnode can containedstored
Floatnodes. This change becomes problematic in theimplementation of
_get_aiida_structure_pymatgen_inlineand_get_aiida_structure_ase_inlineofaiida.tools.data.cif. Theparameterskeyword argument can be an unstoredDictnode, if thefunctions are run with
store_provenance=False. Since it is unstored,it can contain stored nodes for the values within them. For example,
the
site_tolerancecan be a storedFloat. Since they are overloadednative float objects, they can be passed to pymatgens
CifParserwithout it complaining. However, now whenever that class references the
value, since it is a stored node, the
ModelWrapperwill cause themodel instance to be refreshed from the database, which becomes
prohibitively expensive. Simply calling
clean_valueon theparametersbeforehand, will cause these nodes to be dereferenced intotheir base values, which solves the issue.