Skip to content

Commit

Permalink
Fix potential inefficiency in aiida.tools.data.cif converters (#3098)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sphuber committed Jun 30, 2019
1 parent e232f94 commit 1578227
Showing 1 changed file with 10 additions and 4 deletions.
14 changes: 10 additions & 4 deletions aiida/tools/data/cif.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,17 @@
# For further information on the license, see the LICENSE.txt file #
# For further information please visit http://www.aiida.net #
###########################################################################
"""Tools to operate on `CifData` nodes."""
# pylint: disable=invalid-name
"""Tools to operate on `CifData` nodes."""
from __future__ import division
from __future__ import print_function
from __future__ import absolute_import

from six.moves import range

from aiida.orm import CifData
from aiida.engine import calcfunction
from aiida.orm import CifData
from aiida.orm.utils.node import clean_value


class InvalidOccupationsError(Exception):
Expand Down Expand Up @@ -89,7 +91,9 @@ def _get_aiida_structure_ase_inline(cif, **kwargs):
parameters = kwargs.get('parameters', {})

if isinstance(parameters, Dict):
parameters = parameters.get_dict()
# Note, if `parameters` is unstored, it might contain stored `Node` instances which might slow down the parsing
# enormously, because each time their value is used, a database call is made to refresh the value
parameters = clean_value(parameters.get_dict())

parameters.pop('occupancy_tolerance', None)
parameters.pop('site_tolerance', None)
Expand All @@ -115,7 +119,9 @@ def _get_aiida_structure_pymatgen_inline(cif, **kwargs):
parameters = kwargs.get('parameters', {})

if isinstance(parameters, Dict):
parameters = parameters.get_dict()
# Note, if `parameters` is unstored, it might contain stored `Node` instances which might slow down the parsing
# enormously, because each time their value is used, a database call is made to refresh the value
parameters = clean_value(parameters.get_dict())

constructor_kwargs = {}

Expand Down

0 comments on commit 1578227

Please sign in to comment.