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

Add keyword check_occu: bool = True to CifParser.get_structures() #2836

Merged
merged 86 commits into from
Aug 22, 2023

Conversation

jonathanjdenney
Copy link
Contributor

@jonathanjdenney jonathanjdenney commented Feb 7, 2023

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.

@janosh
Copy link
Member

janosh commented Mar 17, 2023

Could you move the test into the existing pymatgen/io/tests/test_cif.py? And add the new kwarg to the doc string? Would be great to have a sentence in there explaining when this might be useful.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 86.66% and project coverage change: -0.58% ⚠️

Comparison is base (ac14c88) 74.63% compared to head (6c32804) 74.06%.

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     
Files Changed Coverage Δ
pymatgen/io/cif.py 92.01% <86.66%> (-0.15%) ⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonathanjdenney
Copy link
Contributor Author

@janosh I believe everything is working as it should now. I added the description you wanted as well.

Comment on lines 1106 to 1113
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
)
Copy link
Member

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?

Copy link
Contributor Author

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.

Comment on lines 1125 to 1136
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

Copy link
Member

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?

@janosh janosh changed the title Add skip_checks to CifParser Add keyword check_occu: bool = True to CifParser.get_structures() Aug 22, 2023
@janosh janosh added enhancement A new feature or improvement to an existing one io Input/output functionality and removed awaiting user Needs more information from OP. labels Aug 22, 2023
Copy link
Member

@janosh janosh left a 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! 👍

@janosh janosh merged commit 0dca5b1 into materialsproject:master Aug 22, 2023
13 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement to an existing one io Input/output functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants