`dials.image_viewer`: add option to display in the "best fit" frame #1716

dagewa · 2021-05-19T15:04:29Z

This should be moved so that it is done once per detector, not every time an image is displayed
It should also be made optional with a checkbox to control

- This should be moved so that it is done once per detector, not every time an image is displayed - It should also be made optional with a checkbox to control

dagewa · 2021-05-19T15:08:42Z

Also, need to check if this breaks the beam centre and other overlays

graeme-winter · 2021-05-19T15:10:22Z

Also, need to check if this breaks the beam centre and other overlays

Checking now

graeme-winter · 2021-05-19T15:11:58Z

@dagewa

Looking good!

graeme-winter · 2021-05-19T15:13:05Z

2 theta > 90° though

Grey-Area fix-image-viewer :( $ dials.image_viewer split_12.*
DIALS (2018) Acta Cryst. D74, 85-97. https://doi.org/10.1107/S2059798317017235
The following parameters have been modified:

input {
  experiments = split_12.expt
  reflections = split_12.refl
}

Traceback (most recent call last):
  File "/Users/graeme/git/dials/build/../modules/dials/command_line/image_viewer.py", line 208, in <module>
    run()
  File "/Users/graeme/git/dials/conda_base/python.app/Contents/lib/python3.8/contextlib.py", line 75, in inner
    return func(*args, **kwds)
  File "/Users/graeme/git/dials/build/../modules/dials/command_line/image_viewer.py", line 204, in run
    show_image_viewer(params=params, reflections=reflections, experiments=experiments)
  File "/Users/graeme/git/dials/build/../modules/dials/command_line/image_viewer.py", line 169, in show_image_viewer
    wrapper.display(experiments=experiments, reflections=reflections)
  File "/Users/graeme/git/dials/modules/dials/util/image_viewer/spotfinder_wrap.py", line 81, in display
    self.frame.load_image(chooser_wrapper(imagesets[0], 0))
  File "/Users/graeme/git/dials/modules/dials/util/image_viewer/spotfinder_frame.py", line 591, in load_image
    super().load_image(
  File "/Users/graeme/git/dials/modules/dials/util/image_viewer/slip_viewer/frame.py", line 423, in load_image
    self.pyslip.tiles.set_image(
  File "/Users/graeme/git/dials/modules/dials/util/image_viewer/slip_viewer/tile_generation.py", line 306, in set_image
    self.flex_image = get_flex_image_multipanel(
  File "/Users/graeme/git/dials/modules/dials/util/image_viewer/slip_viewer/tile_generation.py", line 116, in get_flex_image_multipanel
    beam_center /= npanels / 1e-3
  File "/Users/graeme/git/dials/modules/cctbx_project/scitbx/matrix/__init__.py", line 154, in __truediv__
    return rec([e/other for e in self.elems], self.n)
  File "/Users/graeme/git/dials/modules/cctbx_project/scitbx/matrix/__init__.py", line 154, in <listcomp>
    return rec([e/other for e in self.elems], self.n)
ZeroDivisionError: Please report this error to [email protected]: float division by zero

graeme-winter · 2021-05-19T15:13:33Z

Looking at that one now

graeme-winter · 2021-05-19T15:23:43Z

OK, beam centre nonsense is baked in everywhere which causes massive failures if the two-theta angle is > 90 - because there is no beam centre

However this current state is a long way along the road to fixing. But the rest of the road is a rocky and pothole strewn mudbath, so may be hard going.

phyy-nx · 2021-05-19T18:46:01Z

I re-ran my rotation tests from previous attempts to fix this. Here are the commands again:

dials.import `libtbx.find_in_repositories dials_regression`/image_examples/ALS_831/q315r_lyso_001.img output.experiments=singlepanel.expt
dials.import `libtbx.find_in_repositories dials_regression`/image_examples/LCLS_cspad_nexus/idx-20130301060858401.cbf output.experiments=multipanel.expt
libtbx.python `libtbx.find_in_repositories dials`/util/image_viewer/slip_viewer/rotate_detector.py singlepanel.expt
mv rotated.expt singlepanel_rotated.expt
libtbx.python `libtbx.find_in_repositories dials`/util/image_viewer/slip_viewer/rotate_detector.py multipanel.expt
mv rotated.expt multipanel_rotated.expt

Then use dials.image_viewer on each of the .expt files in turn to see what happens. On both master and this project_2d branch, both single and multi panel images appear rotated correctly.

👍

Also, calculate only when this setting is toggled, not every time an image is displayed. Slightly nasty approach by assigning to an attribute of the detector, but avoids modifying get_flex_image_multipanel signature.

dagewa · 2021-05-19T20:01:57Z

I completed additional work as per the first comment.

It might be a bit messy still. For instance, I tried setting the default to "image" rather than "lab", but it still initially displays the "lab" form. Maybe this is because the calculation is not properly performed until the option is toggled at least once? Anyway, I leave that to anyone with more idea about the image viewer code to look at.

The whole image may shift between the two modes. This is a minor annoyance I don't have time to look at. Finally, as @graeme-winter points out, there are still issues at high angle, which are not due to the best-frame projection but due to problems calculating beam centre.

dagewa · 2021-05-19T20:20:15Z

Demo:

codecov · 2021-05-19T21:13:48Z

Codecov Report

Merging #1716 (a783f29) into main (802c9d6) will increase coverage by 4.96%.
The diff coverage is 74.86%.

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

@@            Coverage Diff             @@
##             main    #1716      +/-   ##
==========================================
+ Coverage   66.65%   71.62%   +4.96%     
==========================================
  Files         615      867     +252     
  Lines       68872    90876   +22004     
  Branches     9572    11314    +1742     
==========================================
+ Hits        45905    65087   +19182     
- Misses      21042    23769    +2727     
- Partials     1925     2020      +95

phyy-nx

LGTM, but I don't have the 2theta tests. On my rotation tests things look good.

graeme-winter · 2021-05-20T07:34:44Z

@dagewa taking a quick look at the default

graeme-winter · 2021-05-20T07:53:45Z

Some confusion in the code...

        self.settings.project_onto = self.params.project_onto

will set it as a string value however

            self.settings.project_onto = self.projection_ctrl.GetSelection()

later on assigns as the index of the selection. I do not think that this feature is unique to this new option.

graeme-winter · 2021-05-20T07:54:30Z

Some confusion in the code...
        self.settings.project_onto = self.params.project_onto
will set it as a string value however
            self.settings.project_onto = self.projection_ctrl.GetSelection()
later on assigns as the index of the selection. I do not think that this feature is unique to this new option.

Fortunately this is never actually read anyway 🙄

dagewa · 2021-05-20T07:57:11Z

Yes, I admit to getting confused by this while copy-and-pasting, but I could not resolve in the time frame. I would welcome anyone with a better idea of what is going on in the image viewer code to tidy up, but I fear starting down that route might end up like an episode of Hoarder Homes.

graeme-winter · 2021-05-20T07:57:34Z

Yes, I admit to getting confused by this while copy-and-pasting, but I could not resolve in the time frame. I would welcome anyone with a better idea of what is going on in the image viewer code to tidy up, but I fear starting down that route might end up like an episode of Hoarder Homes.

Am looking

rjgildea · 2021-05-20T15:37:51Z

The issue is that the attribute detector.projected_2d is only set as a consequence of calling frame.update_settings():

dials/util/image_viewer/slip_viewer/frame.py

Line 497 in a8050e2

self.update_settings()

however this is only called after the image is generated here:

dials/util/image_viewer/slip_viewer/frame.py

Lines 423 to 431 in a8050e2

    
           self.pyslip.tiles.set_image( 
        
               file_name_or_data=img, 
        
               metrology_matrices=self.metrology_matrices, 
        
               get_image_data=get_image_data, 
        
               show_saturated=( 
        
                   self.settings.display == "image" 
        
                   and self.settings.image_type == "corrected" 
        
               ), 
        
           )

Unfortunately the method can't be called before this line, as it appears to depend on attributes defined as part of generating the image...

rjgildea · 2021-05-20T15:41:21Z

The reason it can't be called before the image is created, is because this PR explicitly adds an attribute to self.pyslip.tiles.raw_image.get_detector() which does not exist until self.pyslip.tiles.set_image() has been called:
https://github.com/dials/dials/pull/1716/files#diff-3b34f595b35ea14a7777f614778f9da3c90d11f059de766d5d7d3c4d3ec9ff41R1018-R1025

rjgildea · 2021-05-20T16:19:00Z

Right, so what happens is we call SpotFrame.load_image() which calls XrayFrame.load_image() which calls SpotFrame.update_settings() which is the method that sets the detector.projected_2d attribute. However, update_settings() is only called after calling self.pyslip.tiles.set_image which is what results in self.pyslip.tiles.raw_image being set, which is required by update_settings(), meaning that this can't be called before setting the image.

graeme-winter · 2021-05-20T18:36:58Z

Right, so what happens is we call SpotFrame.load_image() which calls XrayFrame.load_image() which calls SpotFrame.update_settings() which is the method that sets the detector.projected_2d attribute. However, update_settings() is only called after calling self.pyslip.tiles.set_image which is what results in self.pyslip.tiles.raw_image being set, which is required by update_settings(), meaning that this can't be called before setting the image.

So... if we want this to work the only sane option is to evaluate it as part of the image loading. That seems a long way from ideal.

graeme-winter · 2021-05-20T18:38:09Z

I hold the opinion that though this change set is not perfect, the image viewer is a lot better with the changes in than out -> am minded to suggest we merge in it's current form and add a new issue about the annoying behaviour.

I welcome other perspectives.

graeme-winter

Valuable if not perfect improvement, and I appreciate the effort @dagewa made in making these changes. Thank you.

Probably should work something about closing #1715 into the commit messages as this makes it less fundamentally broken.

graeme-winter · 2021-05-20T18:38:53Z

command_line/image_viewer.py

@@ -32,6 +32,8 @@
  .type = int
 color_scheme = *grayscale rainbow heatmap invert
  .type = choice
+project_onto = *lab image


Probably I would prefer projection than project_onto

graeme-winter · 2021-05-20T18:39:55Z

newsfragments/1716.feature

@@ -0,0 +1 @@
+``dials.image_viewer``: Add an option to display in a best-fit "image" coordinate frame.


Probably worth @ndevenish poking at this a little to get some kind of "snapped to grid" idea in there. I have no inspiration otherwise would do myself. Worth making this readable by the general reader.

Indeed, should also now say that the option is the new default

graeme-winter · 2021-05-20T18:40:52Z

util/image_viewer/slip_viewer/frame.py

@@ -524,70 +524,7 @@ def get_key(self, file_name_or_data):
            return super().get_key(file_name_or_data)

    def update_settings(self, layout=True):
-        # XXX The zoom level from the settings panel are not taken into


@rjgildea improving his -ve line count, approve!

This aliasing did completely confuse me while I was trying to change the default, so appreciate it being removed.

That's a couple of haunted trees felled anyway.

graeme-winter · 2021-05-20T18:42:12Z

util/image_viewer/slip_viewer/tile_generation.py

+    # Calculate 2D origin, fast and slow vectors for detector projected onto
+    # a best fit frame. FIXME this should be done once for the detector, not
+    # here for every image we want to display!
+    try:


EAFP is OK, but wonder if this is a little harder to read than just a "hasattr" or even setting the attribute to None when the object is created (which would involve a little more exploration but helps to leave breadcrumbs for a longer term improvement)

oh the FIXME comment is out of date too btw. It came from the first commit where project_2d was called every time this function run rather than checking the detector for pre-calculated results. I did do this in a rush yesterday...

panels is a misnomer here, AFAICT. It is actually a Detector object. We could assign this attribute when the experiments are first loaded in, to ensure it is there whenever it matters.

An alternative is what I suggested at the bottom of cctbx/dxtbx#224 (comment), which is that the project_2d function gets moved to the Detector object itself. Then any detector can tell a viewer how to lay out its panels in 2D. This would have the advantage of providing a method that could be overridden in special cases, like that of the P12M, avoiding the special case code here in the image viewer.

Comment was aimed at the try/except AttributeError pattern, but agree on moving it to Detector as proposed. Could be worth considering if we can do sooner rather than later 🤔

hmm, but actually it would probably have to be in the Format, because I don't think a Detector knows what detector it is...

until then:
projected_axes = getattr(panels, "projected_2d", None)
replaces the 4 lines

Anthchirp · 2021-05-20T20:53:16Z

util/image_viewer/slip_viewer/tile_generation.py

+    # Calculate 2D origin, fast and slow vectors for detector projected onto
+    # a best fit frame. FIXME this should be done once for the detector, not
+    # here for every image we want to display!
+    try:


until then:
projected_axes = getattr(panels, "projected_2d", None)
replaces the 4 lines

Anthchirp · 2021-05-20T20:53:32Z

util/image_viewer/spotfinder_frame.py

+            if not hasattr(detector, "projected_2d"):
+                detector.projected_2d = project_2d(detector)
+            elif detector.projected_2d is None:
+                detector.projected_2d = project_2d(detector)


Suggested change

if not hasattr(detector, "projected_2d"):

detector.projected_2d = project_2d(detector)

elif detector.projected_2d is None:

detector.projected_2d = project_2d(detector)

if not getattr(detector, "projected_2d", None):

detector.projected_2d = project_2d(detector)

☝️ this is kinda what I was getting at 🙂

I appreciate I am not always clear though

get_flex_image_multipanel is also used by export_bitmaps where the detector will not have been furnished with 2d projection (but maybe it should be)

Fixes #1715

graeme-winter · 2021-05-21T09:01:30Z

Following the latest change set @dagewa something is looking good - imported / integrated multi-two-theta offset scan with overlay of reflections:

bonus points - for reasons that are opaque no longer crashes for two theta > 90° 🤔 🤷‍♂️ 👍

dagewa · 2021-05-21T09:20:42Z

I am rather puzzled by the fact that the fix fixes more than I thought it fixed 🤔

Project to 2D "best fit" frame first.

f9d8820

- This should be moved so that it is done once per detector, not every time an image is displayed - It should also be made optional with a checkbox to control

dagewa added 2 commits May 19, 2021 20:43

Add a control to toggle between lab and image projection.

e016e2b

Also, calculate only when this setting is toggled, not every time an image is displayed. Slightly nasty approach by assigning to an attribute of the detector, but avoids modifying get_flex_image_multipanel signature.

News

d96ecfc

dagewa marked this pull request as ready for review May 19, 2021 19:55

dagewa requested review from ndevenish, graeme-winter and phyy-nx May 19, 2021 20:02

dagewa changed the title ~~Project to 2D "best fit" frame first.~~ dials.image_viewer: add option to display in the "best fit" frame May 19, 2021

phyy-nx approved these changes May 19, 2021

View reviewed changes

This method is overriden by subclass, so remove

10d72f7

graeme-winter approved these changes May 20, 2021

View reviewed changes

Change misleading comment

670bf84

Anthchirp reviewed May 20, 2021

View reviewed changes

dagewa added 5 commits May 21, 2021 06:58

project_onto ---> projection

2a121f1

Calculate 2d projection once on setup

463046e

Do still need to test attribute exists.

bb99450

get_flex_image_multipanel is also used by export_bitmaps where the detector will not have been furnished with 2d projection (but maybe it should be)

In practice panels is always a Detector, not merely a panel list

4eb88ee

Change default projection from "lab" to "image"

04c4016

Fixes #1715

Extend projection mode to dials.export_bitmaps

a783f29

Update 1716.feature

f323b1b

ndevenish merged commit 0008175 into main May 21, 2021

ndevenish deleted the project_2d branch May 21, 2021 16:51

dagewa mentioned this pull request May 22, 2021

Disable projection control for P12M #1720

Merged

Anthchirp mentioned this pull request May 25, 2021

dials.image_viewer fundamentally broken if two theta != 0 #1715

Closed

		@@ -0,0 +1 @@
		``dials.image_viewer``: Add an option to display in a best-fit "image" coordinate frame.

dials.image_viewer: add option to display in the "best fit" frame #1716

dials.image_viewer: add option to display in the "best fit" frame #1716

Conversation

dagewa commented May 19, 2021

dagewa commented May 19, 2021

graeme-winter commented May 19, 2021

graeme-winter commented May 19, 2021

graeme-winter commented May 19, 2021

graeme-winter commented May 19, 2021

graeme-winter commented May 19, 2021

phyy-nx commented May 19, 2021

dagewa commented May 19, 2021

dagewa commented May 19, 2021

codecov bot commented May 19, 2021 • edited Loading

Codecov Report

phyy-nx left a comment

Choose a reason for hiding this comment

graeme-winter commented May 20, 2021

graeme-winter commented May 20, 2021

graeme-winter commented May 20, 2021

dagewa commented May 20, 2021

graeme-winter commented May 20, 2021

rjgildea commented May 20, 2021

rjgildea commented May 20, 2021

rjgildea commented May 20, 2021

graeme-winter commented May 20, 2021

graeme-winter commented May 20, 2021

graeme-winter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graeme-winter commented May 21, 2021

dagewa commented May 21, 2021

`dials.image_viewer`: add option to display in the "best fit" frame #1716

`dials.image_viewer`: add option to display in the "best fit" frame #1716

codecov bot commented May 19, 2021 •

edited

Loading