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

Improvements to image stacking #1302

Merged
merged 16 commits into from
Jun 24, 2020
Merged

Improvements to image stacking #1302

merged 16 commits into from
Jun 24, 2020

Conversation

graeme-winter
Copy link
Contributor

Means the images are on a sensible scale when stacked so look OK

Means the images are on a sensible scale when stacked so look OK
@graeme-winter
Copy link
Contributor Author

Before:

Screenshot 2020-06-12 at 16 05 13

adding 10 images =>

Screenshot 2020-06-12 at 16 05 00

After:

adding 10 images gives:

Screenshot 2020-06-12 at 16 05 46

which I think is much more what you would want?

@dagewa
Copy link
Member

dagewa commented Jun 12, 2020

The function name sum_images is not really appropriate if the result is actually to calculate a mean

@graeme-winter
Copy link
Contributor Author

@dagewa valid - but don't want to rename it to compute_mean_over_N_images or something? 🤔

@graeme-winter
Copy link
Contributor Author

Set max brightness to 1000 to reproduce feel of stacking 10 images as before

@graeme-winter graeme-winter marked this pull request as ready for review June 12, 2020 15:17
@graeme-winter
Copy link
Contributor Author

While I am thinking about this, wonder if we should have a show-max mode as well? 🤔 so maximum pixel over the range rather than mean value?

@dagewa
Copy link
Member

dagewa commented Jun 12, 2020

Could definitely be useful - has been used for virtual powder patterns I think

@@ -811,7 +811,7 @@ def draw_resolution_rings(self, unit_cell=None, space_group=None):
update=False,
)

def sum_images(self):
def stack_images(self):
if self.params.sum_images > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to also change the command line parameter to stack_images (possibly keeping around sum_images with a deprecation warning for the time being)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... maybe, provided the people's national bikeshed committee are satisfied with the nomenclature of stacking the images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how to make one phil parameter alias another, and deprecate one or the other, otherwise I would have done this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could replace the name and just break the cl api

Copy link
Contributor

Choose a reason for hiding this comment

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

R.e. marking phil parameters as deprecated:
cctbx/cctbx_project#452 (comment)

I would either:

  1. Rename sum_images to stack_images and re-add sum_images with .deprecated attribute, and ignore. That way it doesn't fail, but users get a warning.
  2. Just rename and accept that very few people likely use the CLI to the image viewer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed change to this effect, if you use old command line option you get

Grey-Area 0 :) $ dials.image_viewer imported.expt strong.refl sum_images=10
DIALS (2018) Acta Cryst. D74, 85-97. https://doi.org/10.1107/S2059798317017235
Sorry: Unknown command line parameter definition: sum_images = 10

which I think is OK

@ndevenish
Copy link
Member

I worry that if summing over a range to try to find weak signal, or signal only present on a few, that this might hide it? A "max pixel" mode might be appropriate. Maybe this does deserve a setting, but maybe that's just avoiding making a decision...

@graeme-winter
Copy link
Contributor Author

I worry that if summing over a range to try to find weak signal, or signal only present on a few, that this might hide it? A "max pixel" mode might be appropriate. Maybe this does deserve a setting, but maybe that's just avoiding making a decision...

My thinking was about simulating a wider image in the first instance, but the max is useful as you say for getting the pattern out. I think the universe has space for both...

Also add a selector pull-down to select preferred stack type. Set max mode
as default since that could well be more useful to the average user
@graeme-winter
Copy link
Contributor Author

@ndevenish added a mean / max pulldown - illustrated difference:

Max:

Screenshot 2020-06-15 at 09 12 18

Mean:

Screenshot 2020-06-15 at 09 12 41

@graeme-winter graeme-winter changed the title If stacking images, divide through Improvements to image stacking Jun 15, 2020
@rjgildea
Copy link
Contributor

As far as I can tell, you can only change the number of images to stack via the command line but not via the GUI, and conversely, you can only change the stack "type" in the GUI but not via the command line. This means that if you load the image viewer with default options, then there is a "stack type" dropdown that doesn't do anything.

The stack_type setting should probably be exposed via the command line, and it only makes really makes sense to have the "stack type" dropdown in the GUI settings panel if you can also control the number of images there too.

@rjgildea
Copy link
Contributor

This seems incompatible with setting the "image type" to be "raw":

$ dials.image_viewer $(dials.data get -q x4wide)/*.cbf stack_images=10

Select "raw" in the "image type" dropdown:

Traceback (most recent call last):
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 2140, in OnUpdateImage
    self.GetParent().GetParent().reload_image()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 513, in reload_image
    self.load_image(self.images.selected, refresh=True)
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 558, in load_image
    show_untrusted=show_untrusted,
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/slip_viewer/frame.py", line 499, in load_image
    self.update_settings()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 1175, in update_settings
    self.stack_images()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 836, in stack_images
    sel = data > rd
TypeError: '>' not supported between instances of 'double' and 'int'

@rjgildea
Copy link
Contributor

Another oddity, this behaviour was probably at least in part already present before this PR - if you're in stack/sum images mode, then press the "image" button, then the displayed image goes back to the individual, rather than stacked, image, until you re-select an item from the "stack type" dropdown menu. I'm not entirely sure what the expected should be for the various thresholding maps - should we be running the threshold algorithms on the stacked image?

@graeme-winter
Copy link
Contributor Author

As far as I can tell, you can only change the number of images to stack via the command line but not via the GUI, and conversely, you can only change the stack "type" in the GUI but not via the command line. This means that if you load the image viewer with default options, then there is a "stack type" dropdown that doesn't do anything.

The stack_type setting should probably be exposed via the command line, and it only makes really makes sense to have the "stack type" dropdown in the GUI settings panel if you can also control the number of images there too.

There is a "Stack" box at the top of the image window? I've been using this extensively so know that works 😉

I had wondered whether the stack_type should be exposed via command line, but not followed up on.

@graeme-winter
Copy link
Contributor Author

This seems incompatible with setting the "image type" to be "raw":

$ dials.image_viewer $(dials.data get -q x4wide)/*.cbf stack_images=10

Select "raw" in the "image type" dropdown:

Traceback (most recent call last):
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 2140, in OnUpdateImage
    self.GetParent().GetParent().reload_image()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 513, in reload_image
    self.load_image(self.images.selected, refresh=True)
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 558, in load_image
    show_untrusted=show_untrusted,
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/slip_viewer/frame.py", line 499, in load_image
    self.update_settings()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 1175, in update_settings
    self.stack_images()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 836, in stack_images
    sel = data > rd
TypeError: '>' not supported between instances of 'double' and 'int'

Reproduced this, will look

@graeme-winter
Copy link
Contributor Author

Another oddity, this behaviour was probably at least in part already present before this PR - if you're in stack/sum images mode, then press the "image" button, then the displayed image goes back to the individual, rather than stacked, image, until you re-select an item from the "stack type" dropdown menu. I'm not entirely sure what the expected should be for the various thresholding maps - should we be running the threshold algorithms on the stacked image?

Thresholding should be fine against summed, mean requires some discussion re: "gain" equivalent, max just plain wrong. Not sure if this should be imposed.

Re: the oddity - the internals of the image viewer are something of a tangled mess. Should fixing this be a necessary precondition of the PR features being made available?

@graeme-winter
Copy link
Contributor Author

This seems incompatible with setting the "image type" to be "raw":

$ dials.image_viewer $(dials.data get -q x4wide)/*.cbf stack_images=10

Select "raw" in the "image type" dropdown:

Traceback (most recent call last):
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 2140, in OnUpdateImage
    self.GetParent().GetParent().reload_image()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 513, in reload_image
    self.load_image(self.images.selected, refresh=True)
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 558, in load_image
    show_untrusted=show_untrusted,
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/slip_viewer/frame.py", line 499, in load_image
    self.update_settings()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 1175, in update_settings
    self.stack_images()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 836, in stack_images
    sel = data > rd
TypeError: '>' not supported between instances of 'double' and 'int'

Reproduced this, will look

Fixed / worked around in c3d54cc

@graeme-winter
Copy link
Contributor Author

As far as I can tell, you can only change the number of images to stack via the command line but not via the GUI, and conversely, you can only change the stack "type" in the GUI but not via the command line. This means that if you load the image viewer with default options, then there is a "stack type" dropdown that doesn't do anything.
The stack_type setting should probably be exposed via the command line, and it only makes really makes sense to have the "stack type" dropdown in the GUI settings panel if you can also control the number of images there too.

There is a "Stack" box at the top of the image window? I've been using this extensively so know that works 😉

I had wondered whether the stack_type should be exposed via command line, but not followed up on.

Done, in a2910e0

@graeme-winter
Copy link
Contributor Author

Another oddity, this behaviour was probably at least in part already present before this PR - if you're in stack/sum images mode, then press the "image" button, then the displayed image goes back to the individual, rather than stacked, image, until you re-select an item from the "stack type" dropdown menu. I'm not entirely sure what the expected should be for the various thresholding maps - should we be running the threshold algorithms on the stacked image?

Thresholding should be fine against summed, mean requires some discussion re: "gain" equivalent, max just plain wrong. Not sure if this should be imposed.

Re: the oddity - the internals of the image viewer are something of a tangled mess. Should fixing this be a necessary precondition of the PR features being made available?

I have looked at the code for a while and can't see where the change should go to make this oddity go away - however since it is not a new oddity I am not too worried about this. I have made sum the default, so if someone stacks images and then looks through the filters the results should be by default meaningful.

@rjgildea
Copy link
Contributor

There is a "Stack" box at the top of the image window? I've been using this extensively so know that works 😉

Ah ok - I had missed that because I was naively expecting related controls to be in the same window at the very least. Does it really make sense for the control for number of images to stack and the control for stack type to be in different windows?

@rjgildea
Copy link
Contributor

I have looked at the code for a while and can't see where the change should go to make this oddity go away - however since it is not a new oddity I am not too worried about this. I have made sum the default, so if someone stacks images and then looks through the filters the results should be by default meaningful.

I'm not convinced the thresholding maps are acting on the sum/mean/max images in any case - I suspect they are effectively switching to stack_images=1 without updating any of the controls to reflect this.

@graeme-winter
Copy link
Contributor Author

There is a "Stack" box at the top of the image window? I've been using this extensively so know that works 😉

Ah ok - I had missed that because I was naively expecting related controls to be in the same window at the very least. Does it really make sense for the control for number of images to stack and the control for stack type to be in different windows?

Respectfully acknowledged. However also mention that stacking images is related to loading images, which is all controlled above the image... and selecting the view kinda lives with the other settings, so maybe (just maybe) the viewer is poorly designed 🤷‍♂️

rd += image_data_i[j]
data = image_data_i[j]
if mode == "max":
if type(rd) != type(data):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can call .as_double() even on an array that is already flex.double(), so you could avoid having to explicitly check the type and always both sides to double.

a = flex.random_double(10).as_double()

@graeme-winter
Copy link
Contributor Author

I have looked at the code for a while and can't see where the change should go to make this oddity go away - however since it is not a new oddity I am not too worried about this. I have made sum the default, so if someone stacks images and then looks through the filters the results should be by default meaningful.

I'm not convinced the thresholding maps are acting on the sum/mean/max images in any case - I suspect they are effectively switching to stack_images=1 without updating any of the controls to reflect this.

Quite possibly: perhaps this is also in the realm of 🤷‍♂️ - since stack images existed before this I am not sure it's a new bug...

@rjgildea
Copy link
Contributor

This seems incompatible with setting the "image type" to be "raw":

$ dials.image_viewer $(dials.data get -q x4wide)/*.cbf stack_images=10

Select "raw" in the "image type" dropdown:

Traceback (most recent call last):
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 2140, in OnUpdateImage
    self.GetParent().GetParent().reload_image()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 513, in reload_image
    self.load_image(self.images.selected, refresh=True)
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 558, in load_image
    show_untrusted=show_untrusted,
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/slip_viewer/frame.py", line 499, in load_image
    self.update_settings()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 1175, in update_settings
    self.stack_images()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 836, in stack_images
    sel = data > rd
TypeError: '>' not supported between instances of 'double' and 'int'

Reproduced this, will look

Fixed / worked around in c3d54cc

This still breaks for me:

$ dials.image_viewer $(dials.data get -q x4wide)/*.cbf stack_images=10
DIALS (2018) Acta Cryst. D74, 85-97. https://doi.org/10.1107/S2059798317017235
The following parameters have been modified:

stack_images = 10
input {
  experiments = <image files>
}

Traceback (most recent call last):
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 2141, in OnUpdateImage
    self.GetParent().GetParent().reload_image()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 513, in reload_image
    self.load_image(self.images.selected, refresh=True)
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 558, in load_image
    show_untrusted=show_untrusted,
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/slip_viewer/frame.py", line 499, in load_image
    self.update_settings()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 1177, in update_settings
    self.stack_images()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 839, in stack_images
    rd += data
Boost.Python.ArgumentError: Python argument types in
    int.__iadd__(int, double)
did not match C++ signature:
    __iadd__(scitbx::af::versa<int, scitbx::af::flex_grid<scitbx::af::small<long, 10ul> > > {lvalue}, int)
    __iadd__(scitbx::af::versa<int, scitbx::af::flex_grid<scitbx::af::small<long, 10ul> > > {lvalue}, scitbx::af::versa<int, scitbx::af::flex_grid<scitbx::af::small<long, 10ul> > >)```

@rjgildea
Copy link
Contributor

Also "max" stack mode + "raw" image type looks very much like it is displaying the single image, and not a stacked image.

@graeme-winter
Copy link
Contributor Author

<- is wondering about abandoning this project as to fix all the problems here will probably require rewriting the image viewer.

@graeme-winter
Copy link
Contributor Author

Also "max" stack mode + "raw" image type looks very much like it is displaying the single image, and not a stacked image.

Sometimes the raw image is ints, sometimes doubles, sometimes the image you are adding is ints, sometimes doubles, and they need to be added in-place so will need to cast images-for-adding to the same type as image-already-there - cannot stick with one "standard" type - so this will be some horrible hodge-podge 😞

Having many random data types for images is plain annoying && unhelpful -
just have doubles to make life simple
@graeme-winter
Copy link
Contributor Author

This seems incompatible with setting the "image type" to be "raw":

$ dials.image_viewer $(dials.data get -q x4wide)/*.cbf stack_images=10

Select "raw" in the "image type" dropdown:

Traceback (most recent call last):
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 2140, in OnUpdateImage
    self.GetParent().GetParent().reload_image()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 513, in reload_image
    self.load_image(self.images.selected, refresh=True)
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 558, in load_image
    show_untrusted=show_untrusted,
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/slip_viewer/frame.py", line 499, in load_image
    self.update_settings()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 1175, in update_settings
    self.stack_images()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 836, in stack_images
    sel = data > rd
TypeError: '>' not supported between instances of 'double' and 'int'

Reproduced this, will look

Fixed / worked around in c3d54cc

This still breaks for me:

$ dials.image_viewer $(dials.data get -q x4wide)/*.cbf stack_images=10
DIALS (2018) Acta Cryst. D74, 85-97. https://doi.org/10.1107/S2059798317017235
The following parameters have been modified:

stack_images = 10
input {
  experiments = <image files>
}

Traceback (most recent call last):
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 2141, in OnUpdateImage
    self.GetParent().GetParent().reload_image()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 513, in reload_image
    self.load_image(self.images.selected, refresh=True)
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 558, in load_image
    show_untrusted=show_untrusted,
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/slip_viewer/frame.py", line 499, in load_image
    self.update_settings()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 1177, in update_settings
    self.stack_images()
  File "/Users/rjgildea/software/cctbx/modules/dials/util/image_viewer/spotfinder_frame.py", line 839, in stack_images
    rd += data
Boost.Python.ArgumentError: Python argument types in
    int.__iadd__(int, double)
did not match C++ signature:
    __iadd__(scitbx::af::versa<int, scitbx::af::flex_grid<scitbx::af::small<long, 10ul> > > {lvalue}, int)
    __iadd__(scitbx::af::versa<int, scitbx::af::flex_grid<scitbx::af::small<long, 10ul> > > {lvalue}, scitbx::af::versa<int, scitbx::af::flex_grid<scitbx::af::small<long, 10ul> > >)```

... try with 372120f

@graeme-winter
Copy link
Contributor Author

I have looked at the code for a while and can't see where the change should go to make this oddity go away - however since it is not a new oddity I am not too worried about this. I have made sum the default, so if someone stacks images and then looks through the filters the results should be by default meaningful.

I'm not convinced the thresholding maps are acting on the sum/mean/max images in any case - I suspect they are effectively switching to stack_images=1 without updating any of the controls to reflect this.

I'm pretty sure this is properly broken. Could disable these buttons if stack != 1? Alternative will be rewriting the way that it works.

@graeme-winter
Copy link
Contributor Author

@rjgildea disabled the buttons if stack now - so behaviour is consistent at least!

@graeme-winter
Copy link
Contributor Author

Summary:

  • using double for all image data (raw; corrected) so internally consistent
  • if stack: disable image filter buttons
  • command-line control of stack number, stack mode
  • tests with raw and corrected images work

Otherwise clicking on the "image" button will take you back to the single image, even
though stack_images is still set to > 1

Also reset the settings.display parameter to "image" in case we were in another
view when entering stack_images mode.
image_data has already been cast to double by get_image_data(). Casting it
again to double at this point means that we are modifying a copy of the
image data instead of the actual data that is displayed.
Copy link
Contributor

@rjgildea rjgildea left a comment

Choose a reason for hiding this comment

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

I've also disabled the "image" button, as clicking on it when in stack mode would reset to displaying a single image, and fixed the "max" stack mode option so that now it actually works.

It now appears to work as expected, and I haven't managed to break it yet 🙂

@graeme-winter graeme-winter merged commit f123a66 into master Jun 24, 2020
@Anthchirp Anthchirp deleted the scale-when-stacking branch October 2, 2020 15:25
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.

4 participants