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

Modified CifParser.check() as one possible solution for issue #3626 #3628

Merged
merged 6 commits into from
Feb 22, 2024

Conversation

kaueltzen
Copy link
Contributor

@kaueltzen kaueltzen commented Feb 16, 2024

closes #3626. CifParser.check() raises a UserWarning: Incorrect stoichiometry for a cif, if

  • the keys _chemical_formula_sum and _chemical_formula_structural are missing and
  • crystallographic sites have different multiplicities

For the relative stoichiometry to be checked correctly, the site multiplicities would have to be included by applying the cif's symmetry operations to the crystallographically unique sites (like for construction for the structure).

As this would be no longer a sanity check, i propose skipping the relative stoichiometry check in this case (but still including all the other checks for valid elements etc.).

For the case of skipping the relative stoichiometry check, i appended a message to self.warnings, but I'm not sure if there are better ways to handle this case.

@kaueltzen kaueltzen marked this pull request as draft February 16, 2024 13:50
@kaueltzen kaueltzen changed the title Modified CifParser.check() as one possible solution for issue #3626 [WIP] Modified CifParser.check() as one possible solution for issue #3626 Feb 16, 2024
@kaueltzen
Copy link
Contributor Author

kaueltzen commented Feb 16, 2024

If you are okay with this approach i could adapt the 2 cif tests that are failing now (and for example add a formula key so the checks are not skipped). (Modifying the tests no longer necessary as other checks than for relative stoichiometry were reincluded in case of missing formula keys)

@kaueltzen kaueltzen changed the title [WIP] Modified CifParser.check() as one possible solution for issue #3626 Modified CifParser.check() as one possible solution for issue #3626 Feb 22, 2024
@kaueltzen kaueltzen marked this pull request as ready for review February 22, 2024 09:53
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 @kaueltzen, this makes sense! also great issue description over in #3626!

if you could add a test with the MCIF file you linked in the issue (or find an existing CIF test file that repros this problem), this is good to go.

pinging @esoteric-ephemera in case he has further comments

if not same_stoich:
failure_reason = f"Incorrect stoichiometry:\n CIF={orig_comp}\n PMG={struct_comp}\n {ratios=}"
else:
self.warnings.append("Skipping relative stoichiometry check because CIF does not contain formula keys.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a test for this warning?

@esoteric-ephemera
Copy link
Contributor

Thanks @kaueltzen for pointing this out! I like your solution of warning that the stoichiometry check couldn't be accomplished - the fallback I added originally clearly isn't robust enough in general

Adding a test for this (using the magnetic cif you sent with the issue) would be great to move this forward

…e of missing formula keys and for appending a warning to self.warnings in this case.
@kaueltzen
Copy link
Contributor Author

Thank you for your quick replies! I added a test both for skipping the relative stoichiometry check and appending a warning in the case of missing formula keys. There was already a test file in pymatgen, Li10GeP2S12.cif, that matched the criteria (it has no formula keys and sites with different multiplicities and occupancies).

@janosh janosh added enhancement A new feature or improvement to an existing one io Input/output functionality ux User experience cif Crystallographic information file labels Feb 22, 2024
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.

very nice work! thanks a lot @kaueltzen 👍

@janosh janosh enabled auto-merge (squash) February 22, 2024 17:54
@janosh janosh merged commit 57842a7 into materialsproject:master Feb 22, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cif Crystallographic information file enhancement A new feature or improvement to an existing one io Input/output functionality ux User experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CifParser check() method does not account for different site multiplicities
3 participants