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 dials.image_viewer crash when the detector swing angle is larger than 90° #2479

Conversation

dagewa
Copy link
Member

@dagewa dagewa commented Jul 31, 2023

Two workarounds required to display images when panel.get_beam_centre_lab raises a RuntimeError, such as in the case of #2478

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!

I confirmed this fixed the problem on several HyPix datasets.

@biochem-fan
Copy link
Member

biochem-fan commented Jul 31, 2023

Wait. I found another related issue.
When indexing results are present, _basis_vector_overlay_data() in spotfinder_frame.py raises an exception.

@biochem-fan
Copy link
Member

diff --git a/src/dials/util/image_viewer/spotfinder_frame.py b/src/dials/util/image_viewer/spotfinder_frame.py
index 1937dcded..d05155e1e 100644
--- a/src/dials/util/image_viewer/spotfinder_frame.py
+++ b/src/dials/util/image_viewer/spotfinder_frame.py
@@ -1952,7 +1952,7 @@ def _basis_vector_overlay_data(self, i_expt, i_frame, experiment):
             if "DXTBX_ASSERT(w_max > 0)" in str(e):
                 # direct beam didn't hit a panel
                 panel = 0
-                beam_centre = detector[panel].get_ray_intersection(beam.get_s0())
+                beam_centre = detector[panel].get_bidirectional_ray_intersection(beam.get_s0())
             else:
                 raise
         beam_x, beam_y = detector[panel].millimeter_to_pixel(beam_centre)

This is a workaround but this shows very long vectors extending beyond the panel. Or should we hide the vectors?
I also don't know the intention of failing back to panel 0.

@dagewa
Copy link
Member Author

dagewa commented Jul 31, 2023

This reminds me that I think something is wrong with the display of vectors in general. In one case I struggled to generate a figure for a paper in which the basis vectors correctly lined up with the spots. In the end I removed them and used the "Show hkl" function instead.

With an additional change you get all 3 vectors rather than only the one that intersects the panel

$ git diff
diff --git a/src/dials/util/image_viewer/spotfinder_frame.py b/src/dials/util/image_viewer/spotfinder_frame.py
index 1937dcded..a81a88306 100644
--- a/src/dials/util/image_viewer/spotfinder_frame.py
+++ b/src/dials/util/image_viewer/spotfinder_frame.py
@@ -1952,7 +1952,7 @@ def _basis_vector_overlay_data(self, i_expt, i_frame, experiment):
             if "DXTBX_ASSERT(w_max > 0)" in str(e):
                 # direct beam didn't hit a panel
                 panel = 0
-                beam_centre = detector[panel].get_ray_intersection(beam.get_s0())
+                beam_centre = detector[panel].get_bidirectional_ray_intersection(beam.get_s0())
             else:
                 raise
         beam_x, beam_y = detector[panel].millimeter_to_pixel(beam_centre)
@@ -1970,8 +1970,8 @@ def _basis_vector_overlay_data(self, i_expt, i_frame, experiment):
                 s1 = matrix.col(beam.get_s0()) + r_phi
             panel = detector.get_panel_intersection(s1)
             if panel < 0:
-                continue
-            x, y = detector[panel].get_ray_intersection_px(s1)
+                panel = 0
+            x, y = detector[panel].get_bidirectional_ray_intersection_px(s1)
             x, y = self.map_coords(x, y, panel)
 
             vector_data.append(

Then setting "Basis scale" to 1 it should show the lengths of the first order reflections, but it clearly does not
image

@dagewa
Copy link
Member Author

dagewa commented Jul 31, 2023

I think the intention of the fallback to panel 0 is so that for cases where the direct beam lands between panels (CS-PAD for example) we can at least draw the blue cross in the panel gap by choosing one of the panel's coordinate systems (at it might as well be the first panel).

In this case the blue cross is well outside the image as a whole. I think it would be reasonable to simply avoid calculating the basis vectors in this case. However, I'm not sure how to test for this case.

@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #2479 (77f614d) into main (77db99a) will decrease coverage by 0.04%.
Report is 1 commits behind head on main.
The diff coverage is 64.67%.

❗ Current head 77f614d differs from pull request most recent head 657208c. Consider uploading reports for the commit 657208c to get more accurate results

@@            Coverage Diff             @@
##             main    #2479      +/-   ##
==========================================
- Coverage   78.65%   78.62%   -0.04%     
==========================================
  Files         606      607       +1     
  Lines       74244    74343      +99     
  Branches    10093    10119      +26     
==========================================
+ Hits        58398    58451      +53     
- Misses      13701    13734      +33     
- Partials     2145     2158      +13     

@biochem-fan
Copy link
Member

biochem-fan commented Jul 31, 2023

With an additional change you get all 3 vectors rather than only the one that intersects the panel

Perhaps this is because with a large $2 \theta$ we are looking at a very curved region of the Ewald sphere and a simple projection into a plane is a bad approximation.
(EDITED: being a sphere, the curvature is constant everywhere)

I think the intention of the fallback to panel 0 is so that for cases where the direct beam lands between panels (CS-PAD for example) we can at least draw the blue cross in the panel gap by choosing one of the panel's coordinate systems (at it might as well be the first panel).

I see.

In this case the blue cross is well outside the image as a whole. I think it would be reasonable to simply avoid calculating the basis vectors in this case. However, I'm not sure how to test for this case.

How about calculating the covex hull of all panels and checking whether the direct beam is within it?

@dagewa
Copy link
Member Author

dagewa commented Jul 31, 2023

How about calculating the covex hull of all panels and checking whether the direct beam is within it?

That sounds good. How about I add your change to this PR to get the image viewer working in this case, and then make a new issue regarding the basis vector display? I think there is more than one problem with it.

Basis vector display is problematic though
@biochem-fan
Copy link
Member

That sounds good. How about I add your change to this PR to get the image viewer working in this case, and then make a new issue regarding the basis vector display? I think there is more than one problem with it.

Yes. That makes sense.

@dagewa dagewa merged commit 4fb2a86 into main Aug 4, 2023
16 checks passed
@dagewa dagewa deleted the 2478-dialsimage_viewer-crashes-when-the-detector-swing-angle-is-larger-than-90 branch August 4, 2023 08:45
toastisme pushed a commit to toastisme/dials that referenced this pull request Aug 21, 2023
…r than 90° (dials#2479)

Fix the image viewer for cases where the detector is in front of the crystal.
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.image_viewer crashes when the detector swing angle is larger than 90
3 participants