NewFeature: Added jump click to plotting

[CSSFrancis]

Description of the change

This is just a quick change to the plotting to allow for jumping. I think we might want to make it possible to change which key is used with the jump.

I can make this a little more robust and we could potentially add it to all of the widgets but the main problem is if you have multiple widgets I don't know how it should work. Same with handing two singles overlaid in 1D.

Progress of the PR

• Change implemented (can be split into several points),
• update docstring (if appropriate),
• update user guide (if appropriate),
• add an changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
• Check formatting changelog entry in the readthedocs doc build of this PR (link in github checks)
• add tests,
• ready for review.

Minimal example of the bug fix or the new feature

%matplotlib ipympl
import hyperspy.api as hs
import dask.array as da
s = hs.signals.Signal2D(np.random.random((100,100,100,100))).as_lazy()
s.plot()

Screencast.from.06-19-2023.06_10_13.PM.online-video-cutter.com.mp4

[codecov]

Codecov Report

Patch coverage: 86.20% and project coverage change: +0.08 🎉

Comparison is base (3e7a7e0) 81.27% compared to head (cbef831) 81.36%.

Additional details and impacted files

@@                  Coverage Diff                   @@
##           RELEASE_next_major    #3175      +/-   ##
======================================================
+ Coverage               81.27%   81.36%   +0.08%     
======================================================
  Files                     176      176              
  Lines                   24401    24485      +84     
  Branches                 5683     5691       +8     
======================================================
+ Hits                    19833    19923      +90     
+ Misses                   3261     3248      -13     
- Partials                 1307     1314       +7     

Impacted Files	Coverage Δ
hyperspy/drawing/_widgets/horizontal_line.py	86.95% <66.66%> (-3.05%)	⬇️
hyperspy/drawing/_widgets/rectangles.py	41.24% <66.66%> (+0.65%)	⬆️
hyperspy/drawing/_widgets/vertical_line.py	91.30% <66.66%> (+1.30%)	⬆️
hyperspy/drawing/widget.py	71.42% <83.33%> (+1.04%)	⬆️
hyperspy/drawing/mpl_he.py	96.93% <100.00%> (+0.01%)	⬆️
hyperspy/misc/test_utils.py	90.27% <100.00%> (+2.14%)	⬆️

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[magnunor]

This is very nice! I tested this a little bit, some comments:

• Currently shift + left and shift + right click "jumps" the navigation position. Should this be the default behavior?
• There is something strange going on: it seems like it is showing another position before jumping to the one I'm clicking. See example below: the "extra" position is the amorphous diffraction pattern. It might be visible here, as this is a lazy dataset being loaded from the hard drive.

• Shift + clicking outside the canvas give an error:

hyperspy/hyperspy/drawing/_widgets/rectangles.py", line 66, in _onjumpclick
    self.position = (event.xdata, event.ydata)
    ^^^^^^^^^^^^^
hyperspy/hyperspy/drawing/widget.py", line 429, in <lambda>
    lambda s, v: s._set_position(v))
                 ^^^^^^^^^^^^^^^^^^
hyperspy/hyperspy/drawing/widget.py", line 423, in _set_position
    position = self._validate_pos(position)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
hyperspy/hyperspy/drawing/widget.py", line 405, in _validate_pos
    pos = np.maximum(pos, [ax.low_value for ax in self.axes])
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: '>=' not supported between instances of 'NoneType' and 'float'

[magnunor]

There is something strange going on: it seems like it is showing another position before jumping to the one I'm clicking. See example below: the "extra" position is the amorphous diffraction pattern. It might be visible here, as this is a lazy dataset being loaded from the hard drive.

This has some performance implications, especially for large lazy datasets, since it means that (potentially) two chunks are loaded into memory for each "jump".

I think this is due to this part of the code:

        if np.ndim(value) == 0 and len(self.axes) == 1:
            self.position = [self.axes[0].index2value(value)]
        elif len(self.axes) != len(value):
            raise ValueError()
        else:
            p = []
            for i in range(len(self.axes)):
                p.append(self.axes[i].index2value(value[i]))
            self.position = p

    indices = property(lambda s: s._get_indices(),
                       lambda s, v: s._set_indices(v))

This part of the code:

hyperspy/hyperspy/drawing/widget.py

Line 363 in d0a266a

def _set_indices(self, value):

[CSSFrancis]

This is very nice! I tested this a little bit, some comments:

Thanks for testing this! Always good to see where things might go wrong on a different computer

• Currently shift + left and shift + right click "jumps" the navigation position. Should this be the default behavior?

I think yes? Unless we want to make the click and drag marker behavior only work with the "left-click". It wouldn't be too hard to do that.

• There is something strange going on: it seems like it is showing another position before jumping to the one I'm clicking. See example below: the "extra" position is the amorphous diffraction pattern. It might be visible here, as this is a lazy dataset being loaded from the hard drive.

This is weird... I'm having a hard time replicating this as well. My best guess here is that your computer is sending a click first to the edge of the canvas and then to the point you actually selected. I've tested this and at least for my computer I don't think it is happening but I'm not sure.

• Shift + clicking outside the canvas give an error:

hyperspy/hyperspy/drawing/_widgets/rectangles.py", line 66, in _onjumpclick
    self.position = (event.xdata, event.ydata)
    ^^^^^^^^^^^^^
hyperspy/hyperspy/drawing/widget.py", line 429, in <lambda>
    lambda s, v: s._set_position(v))
                 ^^^^^^^^^^^^^^^^^^
hyperspy/hyperspy/drawing/widget.py", line 423, in _set_position
    position = self._validate_pos(position)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
hyperspy/hyperspy/drawing/widget.py", line 405, in _validate_pos
    pos = np.maximum(pos, [ax.low_value for ax in self.axes])
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: '>=' not supported between instances of 'NoneType' and 'float'

This is another thing that I'm not getting an error with but regardless it should be fixed by adding the event.inaxes line for some more strict checking.

[CSSFrancis]

There is something strange going on: it seems like it is showing another position before jumping to the one I'm clicking. See example below: the "extra" position is the amorphous diffraction pattern. It might be visible here, as this is a lazy dataset being loaded from the hard drive.

This has some performance implications, especially for large lazy datasets, since it means that (potentially) two chunks are loaded into memory for each "jump".

Yea I think that this is a potential issue but I can't seem to replicate it. My best guess is that it is a bug related to an older matplotlib version and has been fixed?

I think this is due to this part of the code:

        if np.ndim(value) == 0 and len(self.axes) == 1:
            self.position = [self.axes[0].index2value(value)]
        elif len(self.axes) != len(value):
            raise ValueError()
        else:
            p = []
            for i in range(len(self.axes)):
                p.append(self.axes[i].index2value(value[i]))
            self.position = p

    indices = property(lambda s: s._get_indices(),
                       lambda s, v: s._set_indices(v))

This part of the code:

hyperspy/hyperspy/drawing/widget.py

Line 363 in d0a266a

def _set_indices(self, value):

Possibly... I can try it on a different computer/ operating system and see. I wrote some tests so we should be able to test this.

[hakonanes]

Thank you for quickly writing up this PR on discontinuous navigation after our offline discussion, @CSSFrancis!

Plotting geometrical simulations on top of EBSD patterns using markers is helpful when evaluating indexing results. We use this approach extensively in kikuchipy (see this tutorial). My issue with continuous navigation is that all markers are created and added to a signal before navigating. This operation is memory-intensive in kikuchipy. I hope discontinuous navigation can enable lazy markers and computation of one marker at a time.

I tried this PR out locally, and it works nicely! However, I'm seeing the same behavior as @manunor:

it seems like it is showing another position before jumping to the one I'm clicking

[magnunor]

Yea I think that this is a potential issue but I can't seem to replicate it. My best guess is that it is a bug related to an older matplotlib version and has been fixed?

This is with matplotlib version 3.7.1, which is the most recent one. I think the problem is somewhere in HyperSpy. Adding a print for debugging:

    def _onjumpclick(self, event):
        # Callback for MPL pick event
        if event.key=="shift" and event.inaxes:
            print(event.xdata, event.ydata)
            self.position = (event.xdata, event.ydata)

Running this, it only outputs one event per click.

[magnunor]

My guess is that this is only really visible when loading a large dataset from the hard drive, maybe via a HDF5 (.hspy) file.

[hakonanes]

this is only really visible when loading a large dataset from the hard drive, maybe via a HDF5 (.hspy) file.

By 'this', do you mean the extra navigation positing showing? I see this behavior when running @CSSFrancis's example in the top comment, using a Dask array. With Matplotlib v3.7.1.

[magnunor]

Another "bug": shift + clicking on the colorbar also works:

[magnunor]

By 'this', do you mean the extra navigation positing showing? I see this behavior when running @CSSFrancis's example in the top comment, using a Dask array. With Matplotlib v3.7.1.

Yes. I'm running Ubuntu. Could it be a Ubuntu specific issue?

[hakonanes]

I'm also running Ubuntu.

[CSSFrancis]

By 'this', do you mean the extra navigation positing showing? I see this behavior when running @CSSFrancis's example in the top comment, using a Dask array. With Matplotlib v3.7.1.

Yes. I'm running Ubuntu. Could it be a Ubuntu specific issue?

I'm running on Ubuntu 20.04.6 as well with Matplotlib v3.7.1 as well and haven't been able to replicate it. I tried on my M1 macbook as well and couldn't replicate it there either.

[CSSFrancis]

Yea I think that this is a potential issue but I can't seem to replicate it. My best guess is that it is a bug related to an older matplotlib version and has been fixed?

This is with matplotlib version 3.7.1, which is the most recent one. I think the problem is somewhere in HyperSpy. Adding a print for debugging:

    def _onjumpclick(self, event):
        # Callback for MPL pick event
        if event.key=="shift" and event.inaxes:
            print(event.xdata, event.ydata)
            self.position = (event.xdata, event.ydata)

Running this, it only outputs one event per click.

This is the part that I am stuck on... the position is changed which should call the _pos_changed function https://github.com/hyperspy/hyperspy/blob/70ca0762a332a7239a4c1eb75001062ab6c7495d/hyperspy/drawing/widget.py#L382-L393C13

You could try seeing if this function is getting called twice.

It might also be that the self.axes_manager.events.indices_changed.suppress_callback function isn't working properly so the self.axes[i].value = self._pos[i] value is not working properly.

[CSSFrancis]

It might also be that the self.axes_manager.events.indices_changed.suppress_callback function isn't working properly so the self.axes[i].value = self._pos[i] value is not working properly.

That is exactly what is happening! You can kind of see it on the video where one of the indexes switches first and the second one switches a little bit after.

[CSSFrancis]

Okay now try it out... It should be fixed now. Only some of the events where suppressed causing the update function to still trigger. @magnunor good catch!

It was happening on my computer but apparently it was flashing too fast :)

@ericpre do you have any idea if changing from suppress_callback(self._on_navigate) -> supress() will have any larger effects?

[CSSFrancis]

Thank you for quickly writing up this PR on discontinuous navigation after our offline discussion, @CSSFrancis!

Plotting geometrical simulations on top of EBSD patterns using markers is helpful when evaluating indexing results. We use this approach extensively in kikuchipy (see this tutorial). My issue with continuous navigation is that all markers are created and added to a signal before navigating. This operation is memory-intensive in kikuchipy. I hope discontinuous navigation can enable lazy markers and computation of one marker at a time.

@hakonanes Lazy markers is something that I think would be a great thing to add! Most of the groundwork in #3148 is there (I just need to allow a MarkerCollection to accept dask arrays) and I kind of wrote that with the concept of lazy markers in mind.

There are some potential drawbacks of lazy plotting markers (mostly that you can't operate in parallel to calculate them so it could be slow to plot them/ move from one chunk to the next). There are potential ways around that so maybe we start by adding the feature and go from there.

[magnunor]

Seems like there is something strange going on now with the suppression, now it doesn't update the signal:

[CSSFrancis]

Seems like there is something strange going on now with the suppression, now it doesn't update the signal:

Okay maybe I have found the bug but introduced some new ones :) I'm going to have to look into the suppress code and figure out exactly which function to suppress rather than blanket suppressing all of them. It might just be the new jumpclick function.

[sivborg]

Hello, I am also quite fond of this idea, so I can help out with some testing!
I have tried out the implementation on my platform. I use HyperSpy through a Linux server running JupyterHub through TLJH, with %matplotlib widgets used when plotting. I am currently running into the same issues as @magnunor above, but I will try again when there is an update :)

[sivborg]

This fix makes it work as intended on my set-up!

[magnunor]

That fixed the issue with the position not changing (#3175 (comment)), but the "double jump" is still there (the one in the animation in this comment: #3175 (comment))

[CSSFrancis]

Hmmm let me write some more tests and we can go from there.

[CSSFrancis]

I added two tests to make sure that only one index_changed event was occurring as well as a test to make sure that the pick- drag event displays all of the points in-between (only once).

I think that tests the two bugs that we were seeing here.

[magnunor]

That did indeed fixed the issue! Now it works nicely, and there is one "jump" per click.

[CSSFrancis]

That did indeed fixed the issue! Now it works nicely, and there is one "jump" per click.

Thanks for all of your testing and finding some of the bugs I missed :). Probably should have just written the event tests first. There also should probably be more mock event tests in general so this is probably a good start.

@ericpre when you have a chance, can you look at how I handled the event suppression before this is merged?

[magnunor]

Thanks for all of your testing and finding some of the bugs I missed :). Probably should have just written the event tests first. There also should probably be more mock event tests in general so this is probably a good start.

Yeah, I find that for some features or changes that is a nice workflow: adding the tests first, then implementing the change.

[ericpre]

This is useful., however, this is working only the rectangle widget. It shouldn't too difficult to make it work for all widgets.

[ericpre]

Instead to making a SquareWidget._pos_changed the code should be changed in DraggableWidgetBase._pos_changed?

[CSSFrancis]

This is useful., however, this is working only the rectangle widget. It shouldn't too difficult to make it work for all widgets.

Yea I think that it would be good. I can add it for the VerticalLine class as well. Working for all widgets might be a little more complicated when you have multiple widgets on one plot. I'll look into it and see what I can do.

[ericpre]

Indeed, the several widgets situation needs to be handle correctly, currently, when adding several square widgets, the issue will occur. The widgets have a selected attribute, which could be used to know if the widget needs to be moved or not: move the widget only when selected is True and there should be only one widget selected a time. However, I wouldn't be surprised if it is a bit buggy and an alternative would be to keep this functionality private and enable it only when used as pointer and come back to this in a separate PR.

[CSSFrancis]

@ericpre I added in the ability to use the jump-click method with the Horizontal and Vertical plots and refactored things based on your suggestions. I also added some tests for testing the dragging a Line Marker

[ericpre]

That's brilliant! Just need some clean-up.

[ericpre]

It seems that the very first event with shift key press is ignored if no previous matplotlib event was triggered. It may well come from matplotlib side: for example, when the figure is resized before trying triggering a matplotlib event, then it works as expected (first event not ignored) and this is suspicious!

[CSSFrancis]

It seems that the very first event with shift key press is ignored if no previous matplotlib event was triggered. It may well come from matplotlib side: for example, when the figure is resized before trying triggering a matplotlib event, then it works as expected (first event not ignored) and this is suspicious!

I think this is just because the figure needs to be active/ selected? The same thing applies to using the arrow keys where you need to have clicked on the figure first. It's kind of frustrating but I'm not really sure how you would fix that?

I'd have to dig into the event.inaxes but I suspect that the first click doesn't trigger event.inaxes.

[ericpre]

I think this is just because the figure needs to be active/ selected? The same thing applies to using the arrow keys where you need to have clicked on the figure first. It's kind of frustrating but I'm not really sure how you would fix that?

Yes, I thought so too but if this was the case, it would do it too for the click and drag of the pointer, which works fine at first attempt. For some reason, shift is ignore until the window is selected while other event are triggered fine... This is suing qt and widget backends on windows, I haven't tested other platform.
I can't think of a reason why it would be related to this PR, so this is good as it is!