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 --double-wilson-reindexing-ops #154

Merged
merged 4 commits into from
Apr 30, 2024
Merged

add --double-wilson-reindexing-ops #154

merged 4 commits into from
Apr 30, 2024

Conversation

kmdalton
Copy link
Member

@kmdalton kmdalton commented Mar 4, 2024

This PR adds the option to provide reindexing operations for edges in the Double Wilson prior graph.

@DHekstra
Copy link

DHekstra commented Mar 5, 2024

what could be useful indicators that the symop the user provides is correct? Perhaps % children with a parent?

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 20.00000% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 78.47%. Comparing base (0d5e4a4) to head (ee226e7).

❗ Current head ee226e7 differs from pull request most recent head 2147ca8. Consider uploading reports for the commit 2147ca8 to get more accurate results

Files Patch % Lines
careless/io/manager.py 16.66% 5 Missing ⚠️
careless/models/priors/wilson.py 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #154      +/-   ##
==========================================
- Coverage   78.69%   78.47%   -0.23%     
==========================================
  Files          52       52              
  Lines        2347     2355       +8     
==========================================
+ Hits         1847     1848       +1     
- Misses        500      507       +7     
Flag Coverage Δ
unittests 78.47% <20.00%> (-0.23%) ⬇️

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.

@kmdalton
Copy link
Member Author

kmdalton commented Mar 5, 2024

I don't think the % of child reflections which can be mapped into the parent ASU is diagnostic. I can think of correct examples where the number will be at most 50% and incorrect examples where it could be 100%. I think that validating the choice of symops is outside of the scope of what I can do inside the careless CLI. I would think that CCsym is probably the most diagnostic reciprocal space measure.

@DHekstra
Copy link

OK, I am fine with merging this and closing the corresponding issue. The implementation is sound. The tricky parts (using the correct symops and properly treating the Miller indices-as-metadata) are external to Careless.

@kmdalton kmdalton merged commit 9940115 into main Apr 30, 2024
4 checks passed
@kmdalton kmdalton deleted the reindexing_ops branch April 30, 2024 21:01
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