Various bug and consistency fixes for CifData and StructureData
#2374
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
sphuber
force-pushed
the
fix_2373_cif_has_partial_occupancies
branch
from
508d9d7 to
b78e2bd
Compare
sphuber
force-pushed
the
fix_2373_cif_has_partial_occupancies
branch
from
b78e2bd to
524edee
Compare
For `CifData` with values of the `_atom_site_occupancy` tag that cannot be parsed into a float, the `has_partial_occupancies` property would except. Here we properly check whether the value can be cast to a float after removing any parentheses that may be present.
The `CifData.has_unknown_species` return whether the chemical formula contains any unknown species, which were defined as those species that are not listed in `aiida.common.constants.elements`. This was intended to filter out those cif files that contained species that could not be parsed into a `StructureData` that could actually be used for a condensed matter calculation, since the presence of an unknown species would render that impossible. However, recently, the specie `X` was added to the internal dictionary of elements, breaking the method. Here we explicitly ignore `X` in the `has_unknown_species` method, returning its implementation to its original intention.
This property will verify that all coordinates of all atomic sites can be properly converted into float values. If just one coordinate cannot be successfully converted to a float, the cif is said to have unknown atomic sites, as it will not be able to be parsed into a usable `StructureData` object for further calculations.
The `Kind` and `StructureData` were using an inconsisten mixture of methods and properties for boolean attributes, such as `has_vacancies` and `is_alloy`. To conform to `CifData` these are now all turned into properties.
giovannipizzi
approved these changes
Jan 9, 2019
sphuber
force-pushed
the
fix_2373_cif_has_partial_occupancies
branch
from
524edee to
23895b2
Compare
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 #2373
Bug fixes:
CifData.has_partial_occupanciesCifData.has_unknown_speciesAdded properties:
CifData.has_unknown_atomic_sitesConsistency changes:
Kind.is_alloy-> propertyKind.has_vacancies-> propertyStructureData.is_alloy-> propertyStructureData.has_vacancies-> property