Skip to content
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

Potential inefficiency in aiida.tools.data.cif CifData converters #3097

Closed
sphuber opened this issue Jun 28, 2019 · 0 comments · Fixed by #3098
Closed

Potential inefficiency in aiida.tools.data.cif CifData converters #3097

sphuber opened this issue Jun 28, 2019 · 0 comments · Fixed by #3098

Comments

@sphuber
Copy link
Contributor

sphuber commented Jun 28, 2019

In a recent commit, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant