Add ability to build out unit cells from CIF files #11
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
DomFijan
reviewed
Jun 10, 2024
Comment on lines
+248
to
+250
| _, unique_indices, unique_counts = np.unique( | ||
| pos.round(n_decimal_places), return_index=True, return_counts=True, axis=0 | ||
| ) |
There was a problem hiding this comment.
Would it be possible to use tolerance instead of rounding here? This is OK, but rounding doesn't give you precise control over the tolerance you wish to apply....
There was a problem hiding this comment.
I will come up with a solution for this - it's a shame it's not implemented in Numpy general, as this is a problem I run into frequently.
janbridley
commented
Jun 10, 2024
janbridley
commented
Jun 10, 2024
janbridley
commented
Jun 10, 2024
janbridley
force-pushed
the
feature/supercells
branch
from
fad06db to
d50b3ee
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.
Description
A primary use case of this package is building out crystal structures. CIF files store this data as Wyckoff (symmetry irreducible) sites and symmetry operations. The
extract_atomic_positionsfunction applies every symmetry operation to every Wyckoff site and filters out duplicate points (after wrapping the coordinates back into the box).Caveats
As a text format, CIF makes no guarantees about the precision of numerical data. Files with 3 or fewer decimal places may result in sites that are effectively overlapping but are not pruned by the uniqueness check. This is the case for gemmi and ASE as well, although this function gets the correct number of atoms more frequently than gemmi. With enough decimal places, all of these packages get the same result.
The uniqueness check
We perform two (array-set) operations to filter unique points: one on the fractional coordinates, and one on the coordinates in real space. The fractional coordinate check removes about 96% of duplicate points (based on an example 1000 files from the AFlow database). The second check matches another ~1% of coordinates, for a total of 97% accuracy. Using a distance based check (as in gemmi and ase) yields similar statistics but more cases where too many points are removed. In my opinion this is worse, as it's far more difficult to add in unmatched points than it is to manually remove near-duplicates. Either way, it seems like Parsnip is at least as effective as established alternatives for this method.
Types of Changes
1The change breaks (or has the potential to break) existing functionality and should be merged into the
breakingbranchChecklist: