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

Improve error handling in SymmetryData #1118

Merged
merged 6 commits into from
Oct 1, 2024

Conversation

mkhorton
Copy link
Member

Hi everyone, just a small PR:

The SymmetryData model currently has optional fields, and therefore can be constructed even if symmetry information is unknown, e.g. if spglib fails.

This PR improves error handling in .from_structure to gracefully handle cases where spglib cannot determine the symmetry of a structure. A common use case where this might be encountered is if an otherwise valid StructureMeta (or a subclass of StructureMeta) is instantiated, but with symmetry information unknown.

The PR also improves handling of angle_tolerance to ensure that this is picked up from EmmetSettings instead of assuming the default value, and records this alongside symprec in the document model. This is also an optional field so as not to break backwards compatibility.

It might be better to make these fields non-optional, but that would be a breaking change and I don't want to suggest it in this PR.

@tsmathis
Copy link
Collaborator

Awesome, thanks @mkhorton. Is there a reason for the minimum pymatgen version? And if you can fix the linting that would be great.

And apologies for the failing workflows for the tests., we haven't configured the enum generation workflow to be compatible with forks. I've ran the tests locally with your patch and everything looks good.

@tsmathis tsmathis added the Core Any updates for Emmet-Core label Sep 28, 2024
@mkhorton
Copy link
Member Author

No worries about the tests from my side, I did test this myself also.

For the minimum pymatgen version, this is because SymmetryUndeterminedError was only added back in June. Previously this kind of error wasn't handled well in pymatgen either, for example Structure.get_spacegroup_info() would raise an odd KeyError if symmetry wasn't determined because the symmetry dataset dict would be None. But in either case, the minimum pymatgen version set is already an earlier than the versions pinned in the requirements/ files, so it should be ok.

@tsmathis
Copy link
Collaborator

Sounds good, just wanted to be sure there's a reason if we're asked in the future.

@mkhorton
Copy link
Member Author

Just ran through black, hopefully that fixes the linter issues. I don't actually have a dev environment set up for emmet right now otherwise, this is a bit of a hot fix :)

provide -> provided
@tsmathis
Copy link
Collaborator

All good, pre-commit complains a lot.

You happy with this? Can merge whenever you're ready

@mkhorton
Copy link
Member Author

mkhorton commented Oct 1, 2024

I'm happy with this, thanks Tyler!

@tsmathis tsmathis merged commit 96ad05a into materialsproject:main Oct 1, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Any updates for Emmet-Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants