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

Multi-panel and multi-experiment dials.rs_mapper #2362

Merged
merged 13 commits into from
Mar 14, 2023
Merged

Conversation

dagewa
Copy link
Member

@dagewa dagewa commented Mar 7, 2023

Allow dials.rs_mapper to work for multi-panel detectors by looping over panels at the highest level and accumulating the results. So far I tested only for a very simple 4 panel, small molecule MicroED dataset and the results appear sensible.

image

This should close #2333.

@dagewa dagewa requested a review from biochem-fan March 7, 2023 16:03
I think this restriction is artificial. Whether or not a multi-
experiment calculation is meaningful depends on context, but we
can leave that up to the user to decide.
@dagewa dagewa changed the title Multi-panel dials.rs_mapper Multi-panel and multi-experiment dials.rs_mapper Mar 8, 2023
@dagewa
Copy link
Member Author

dagewa commented Mar 8, 2023

In addition, I removed the restriction to operate over one experiment only. There may be situations like multi sweep datasets where constructing the reciprocal lattice over all sweeps is interesting.

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #2362 (09e3d0b) into main (945479f) will decrease coverage by 0.02%.
The diff coverage is 52.00%.

❗ Current head 09e3d0b differs from pull request most recent head 63fa54f. Consider uploading reports for the commit 63fa54f to get more accurate results

@@            Coverage Diff             @@
##             main    #2362      +/-   ##
==========================================
- Coverage   82.83%   82.82%   -0.02%     
==========================================
  Files         593      593              
  Lines       68601    68612      +11     
  Branches     9222     9221       -1     
==========================================
  Hits        56828    56828              
- Misses       9656     9668      +12     
+ Partials     2117     2116       -1     

Copy link
Member

@biochem-fan biochem-fan left a comment

Choose a reason for hiding this comment

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

Thank you very much for enhancement. The changes look OK to me.

@biochem-fan
Copy link
Member

I think this restriction is artificial. Whether or not a multi-experiment calculation is meaningful depends on context, but we can leave that up to the user to decide.

How about adding an option to rotate the experiment according to the orientation matrix, when given
indexed.expt, not imported.expt? This might be useful when pathology (e.g. streaking) is suspected
in a serial or small-wedge crystallography dataset.

(Just an idea for future enhancement, not a requirement for this pull request.)

@dagewa
Copy link
Member Author

dagewa commented Mar 9, 2023

Thanks, that sounds useful. In fact, perhaps that should be default if multiple experiments are provided that have crystal models. I might still put it in here - I can't merge this yet because we're getting failures with I23 data and it isn't clear why.

@dagewa dagewa marked this pull request as draft March 9, 2023 10:08
@dagewa dagewa marked this pull request as ready for review March 14, 2023 14:27
AFAICT there was not one on dials-data, so I used an I23 image from
dials_regression, which is already used in other tests.
@dagewa dagewa enabled auto-merge (squash) March 14, 2023 14:52
@dagewa dagewa merged commit 6f8cd58 into main Mar 14, 2023
@dagewa dagewa deleted the multi-panel-rs-mapper branch March 14, 2023 18:49
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.

dials.rs_mapper does not work for multi-panel detectors
3 participants