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

Fix is_isomorphous #279

Merged
merged 9 commits into from
Nov 4, 2024
Merged

Fix is_isomorphous #279

merged 9 commits into from
Nov 4, 2024

Conversation

kmdalton
Copy link
Member

@kmdalton kmdalton commented Nov 4, 2024

As discussed in #277 , DataSet.is_isomorphous currently does not do what is indicated in its docstring. This change makes the behavior in line with the expectation that cell_threshold is a percent difference.

@kmdalton kmdalton requested a review from tjlane November 4, 2024 16:21
@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.98%. Comparing base (b78e571) to head (2222d77).
Report is 64 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #279      +/-   ##
==========================================
- Coverage   91.22%   88.98%   -2.25%     
==========================================
  Files          37       40       +3     
  Lines        2531     2841     +310     
==========================================
+ Hits         2309     2528     +219     
- Misses        222      313      +91     
Flag Coverage Δ
unittests 88.98% <100.00%> (-2.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Collaborator

@tjlane tjlane left a comment

Choose a reason for hiding this comment

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

Bug is fixed.

I'd like to see a (regression) test for this method added before merging, to ensure it stays fixed.

@kmdalton kmdalton requested a review from tjlane November 4, 2024 18:54
@kmdalton
Copy link
Member Author

kmdalton commented Nov 4, 2024

as requested, @tjlane

Copy link
Collaborator

@tjlane tjlane left a comment

Choose a reason for hiding this comment

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

Nice! Logic looks solid. A few comments in the interest of people who might read this code in the future.

Also: do we need a test for the spacegroup comparison bit?

tests/test_dataset.py Outdated Show resolved Hide resolved
tests/test_dataset.py Outdated Show resolved Hide resolved
@kmdalton
Copy link
Member Author

kmdalton commented Nov 4, 2024

@tjlane , space groups are tested in test_is_isomorphous. I don't think there is any point in doing it for this test too.

@kmdalton kmdalton requested a review from tjlane November 4, 2024 22:08
Copy link
Collaborator

@tjlane tjlane left a comment

Choose a reason for hiding this comment

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

love it

@tjlane
Copy link
Collaborator

tjlane commented Nov 4, 2024

thanks for tackling this one @kmdalton !

@kmdalton kmdalton merged commit e2b70ad into main Nov 4, 2024
10 checks passed
@kmdalton kmdalton deleted the fix_is_isomorphous branch November 4, 2024 22:19
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