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 convenience util func for generating robocrys descriptions #1076

Merged
merged 7 commits into from
Sep 12, 2024

Conversation

tsmathis
Copy link
Collaborator

@esoteric-ephemera, any extra additions when you get a chance.

Likely need some flexibility with kwargs, or any defaults you think are sane.

@tsmathis
Copy link
Collaborator Author

tsmathis commented Sep 4, 2024

@esoteric-ephemera, can you take a look at this when you have a chance? Not sure if this still applies with the recent robocrys updates

@esoteric-ephemera
Copy link
Collaborator

Yes! Have some specific thoughts right now having run this a bunch: there are a few common exceptions that get thrown by robocrys that I don't think we're handling:

  • 'Symmetry detection failed for structure with formula < formula >. Try setting symprec=0.01 to a different value.'
  • "2D components don't all have the same orientation."
  • 'No Voronoi neighbors found for site - try increasing cutoff'
  • 'No unicode character exists for subscript period.'

I think we can maybe treat the first one by adjusting symprec thru the kwargs, the last one (missing unicode character) probably needs a fix on the robocrys side

The Voronoi exception is from pymatgen, might be too buried to adjust through robocrys kwargs but I'll look into that

Will also add some defaults logic - we probably want some additions to the builder too to reduce load time in mineral matcher

@tsmathis
Copy link
Collaborator Author

tsmathis commented Sep 4, 2024

All sounds good

From the builder side the plan was just to pass in an already instantiated mineral matcher to the from_structure method, which can be reused in further calls to from_structure.

Just instance like mineral_matcher = MineralMatcher(), should there be anything different there?

@esoteric-ephemera
Copy link
Collaborator

@tsmathis sorry for the delay, adding some extra logic for handling some common exceptions - think this is in good shape

@tsmathis
Copy link
Collaborator Author

nice, thanks!

@tsmathis
Copy link
Collaborator Author

tsmathis commented Sep 12, 2024

one thing, since robocrys is optional for emmet how do you want to handle the MineralMatcher imports? In the scope of this the MineralMatcher is only imported for type hinting

@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 21.42857% with 22 lines in your changes missing coverage. Please review.

Project coverage is 90.18%. Comparing base (1d490e0) to head (a858ee8).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
emmet-core/emmet/core/utils.py 12.50% 21 Missing ⚠️
emmet-core/emmet/core/robocrys.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1076       +/-   ##
===========================================
+ Coverage   72.66%   90.18%   +17.52%     
===========================================
  Files          77      147       +70     
  Lines        5015    14174     +9159     
===========================================
+ Hits         3644    12783     +9139     
- Misses       1371     1391       +20     

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

@esoteric-ephemera
Copy link
Collaborator

one thing, since robocrys is optional for emmet how do you want to handle the MineralMatcher imports? In the scope of this the MineralMatcher is only imported for type hinting

Since we're using the legacy type hints, I think your solution (just removing the type hint for mineral_matcher) is OK!

@tsmathis tsmathis merged commit a6c86e0 into main Sep 12, 2024
7 checks passed
@tsmathis tsmathis deleted the robocrys-utils branch September 12, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants