-
Notifications
You must be signed in to change notification settings - Fork 874
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
Add keyword check_occu: bool = True
to CifParser.get_structures()
#2836
Add keyword check_occu: bool = True
to CifParser.get_structures()
#2836
Conversation
for more information, see https://pre-commit.ci
add tests to skip_checks
…pymatgen into Add-Skip_checks
for more information, see https://pre-commit.ci
…pymatgen into Add-Skip_checks
for more information, see https://pre-commit.ci
Could you move the test into the existing |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2836 +/- ##
==========================================
- Coverage 74.63% 74.06% -0.58%
==========================================
Files 230 230
Lines 69403 69426 +23
Branches 16161 16169 +8
==========================================
- Hits 51802 51422 -380
- Misses 14528 14959 +431
+ Partials 3073 3045 -28
☔ View full report in Codecov by Sentry. |
…pymatgen into Add-Skip_checks
@janosh I believe everything is working as it should now. I added the description you wanted as well. |
pymatgen/io/cif.py
Outdated
if skip_occu_checks: | ||
struct_2 = Structure( | ||
lattice, all_species, all_coords, site_properties=site_properties, labels=all_labels | ||
) | ||
for idx in range(len(struct_2)): | ||
struct_2[idx] = PeriodicSite( | ||
all_species_noedit[idx], all_coords[idx], lattice, properties=site_properties, skip_checks=True | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks needlessly complicated. What's the reason for creating struct_2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason struct_2 was there is no longer in cif.py. I've updated the file so that it doesn't make a new object.
pymatgen/io/cif.py
Outdated
if skip_occu_checks: | ||
struct_2 = SymmetrizedStructure(struct, sg, equivalent_indices, wyckoffs) | ||
for idx in range(len(struct_2)): | ||
struct_2[idx] = PeriodicSite( | ||
all_species_noedit[idx], | ||
all_coords[idx], | ||
lattice, | ||
properties=site_properties, | ||
skip_checks=True, | ||
) | ||
return struct_2 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Can this be simpler?
check_occu: bool = True
to CifParser.get_structures()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for persisting on this PR @jonathanjdenney! 👍
If
check_occu=False
, site occupancy will not be checked, allowing unphysical occupancy != 1. Useful for experimental results in which occupancy was allowed to refine to unphysical values. Warning: unphysical site occupancies are incompatible with many pymatgen features.