PolygonROI

[sivborg]

This is part of the implementation of #2993 , where the aim is to integrate GUI masking into HyperSpy from matplotlib widgets.

As a step forward, I have extended the ROI functionality to a two-dimensional polygonal region-of-interest. Since this step is independent of the GUI, and to keep the PRs in more manageable scopes, I decided to merge this functionality in first.

Moreover, this feature reuses a lot of code from CircleROI as the principle is very similar.

Since the masking tools are targeted for RELEASE_next_major, this PR is as well. However, this would also be suitable for a minor release, so I can close this PR and move the functionality to target RELEASE_next_minor if preferred.

While this PR has some masking functionality, I do not want it to interfere with #2375, so let me know if I should

Description of the change

• Added a PolygonROI region-of-interest class that can be used to mask areas as defined by a polygon
• Added to it a boolean_mask function that can retrieve the ROI as a boolean numpy array, which can be passed on to methods requiring a mask.

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

import hyperspy.api as hs
import numpy as np
s = hs.signals.Signal1D(np.ones((50, 60, 4)))
polygon_roi = hs.roi.PolygonROI([(20, 20), (36, 20), (26, 36)])
masked = polygon_roi(s)

Technical details.

The implementation is very similar to CircleROI, but uses a mask from rasterizing a polygon rather than a circle.

As a polygon is not directly available as a widget in HyperSpy, I have omitted the interactive functionality for now. As the functionality is added to the GUI, PolygonROI can be extended to support this.

The algorithm used to rasterize the polygon into an array has a few limitations:

• A self-overlapping polygon can have its overlapping areas masked out, which can easily be unintended. I have tried to make this clear in the docstrings.
• The algorithm is implemented with Python built-ins, meaning it can be slow to run it repeatedly. One way to remedy this is to run the algorithm every time the polygon is updated, then cache the resulting array as a member. I'm not sure whether this efficiency is worth it, however.

Compared to other ROIs, this one does not have a fixed amount of parameters as a polygon can have as many points as necessary. This changes some of the conventions by displaying the parameters. Currenly, this is how the class behaves:

r = hs.roi.PolygonROI([(10, 20),(20, 50),(30, 30)])
print(r.parameters)
print(tuple(r))
print(repr(r))

{'points': [(10, 20), (20, 50), (30, 30)]}
((10, 20), (20, 50), (30, 30))
PolygonROI(points=[(10, 20), (20, 50), (30, 30)])

[codecov]

Codecov Report

Attention: Patch coverage is 99.27007% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.37%. Comparing base (87b61b6) to head (b443458).
Report is 75 commits behind head on RELEASE_next_minor.

Files with missing lines	Patch %	Lines
hyperspy/drawing/_widgets/polygon.py	97.33%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #3030      +/-   ##
======================================================
+ Coverage               81.12%   81.37%   +0.24%     
======================================================
  Files                     146      147       +1     
  Lines                   22129    22403     +274     
  Branches                 4934     5002      +68     
======================================================
+ Hits                    17953    18230     +277     
+ Misses                   2989     2984       -5     
- Partials                 1187     1189       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ericpre]

From a quick look, this seems to be on the right track. I suspect that the most difficult part is to generate the mask, which is already done here. Do you wan to add the matplotlib polygon widget in this PR? I think that it shouldn't be too much code.

Since the masking tools are targeted for RELEASE_next_major, this PR is as well. However, this would also be suitable for a minor release, so I can close this PR and move the functionality to target RELEASE_next_minor if preferred.

I think that it would be unlikely that they will be a minor release between the major release.

While this PR has some masking functionality, I do not want it to interfere with #2375, so let me know if I should

As I understand, even if both PR are dealing with mask, their focus are different.

The implementation is very similar to CircleROI, but uses a mask from rasterizing a polygon rather than a circle.

I guess that it would be good to make a base class for mask-ROI classes, as I would imagine that in the future, they could be more mask-ROI classes?

[sivborg]

Thanks for the look!

Do you wan to add the matplotlib polygon widget in this PR? I think that it shouldn't be too much code.

I think I can draft the implementation of this pretty soon and decide after the scope of it is clear.

I guess that it would be good to make a base class for mask-ROI classes, as I would imagine that in the future, they could be more mask-ROI classes?

Yes, there is a bit of shared code that could be useful to put in its own class. It might have to involve multiple inheritance, as CircleROI inherits from the interactive ROI while PolygonROI does not. So I think I will examine it a bit further, first.

[sivborg]

Though I had some difficulties, I have finished a working prototype for integrating matplitlob.widgets selectors into HyperSpy, which here is done in the PolygonWidget class. The code requirements were actually not very demanding, so I believe I can fit it into this PR.

The interface for the MPL widgets are quite different from the existing HyperSpy ones, so I added a base class MPLWidgetBase that implements some common operations, so we can expand it to other widgets in matplotlib.widgets if desired. However, it might be a good idea to create a common base class between WidgetBase and MPLWidgetBase. I will look a bit into it.

[sivborg]

Hello, I currently have a working implementation of the PolygonROI and accompanying PolygonWidget. I ended up spending quite some more time developing this, as I found it useful the option of having multiple polygons as a ROI, which is now implemented. Let me know what you think!

Here is a minimal example of how it works, with get_dummy_signal as defined at the end of this comment:

import hyperspy.api as hs
import hyperspy.drawing.widgets as widgets

s = get_dummy_signal()
s.plot()

roi = hs.roi.PolygonROI()
roi.add_widget(s, axes=[2,3]) # Leave out `axes` argument for navigation space

s_roi = roi(s, axes=[2,3])
s_roi.plot()

"Escape" resets the currently constructed polygon, "shift"+click to move it, grab the vertex handles to move the vertices. Click outside the finished polygon to start another one.
Click inside a finished polygon to select it, giving it a thick red outline. Then press "Delete" to remove the selected one. This can not be done while constructing another one.

You can also get a boolean mask from the ROI, which is useful for interfacing with other libraries:

mask = roi.boolean_mask(s.axes_manager,axes=[2,3])

I have tested that it works with the same interface/behaviour as the other ROIs, in particular CircleROI which is quite similar. I have checked and found it seems to work with .interactive and lazy signals, although not together as much. It works with IPython too, I've found. Also with different scale factors on x- and y-axis.
However, it does not currently have a .gui() implementation.

Here is an example of using it interactively with a histogram:

s = get_dummy_signal()
s.plot()

roi = hs.roi.PolygonROI([[10, 20], [14, 13], [1, 10]]) # You can supply initial polygons as lists of vertices.
roiint = roi.interactive(s,axes=[2,3])

roihist = hs.interactive(roiint.get_histogram,
                        event=roi.events.changed,
                        bins=150,
                        recompute_out_event=None
)
roihist.plot()

The public interface of matplotlib.widgets.PolygonSelector, which this functionality is based on, does not expose much of its behavior. So the implementation is a combination of basic matplotlib together with PolygonSelector.

Hope this will be of use, and thank you for any feedback! There are perhaps some things still that I would like to go over once more. Plus some tests.

(Here is my example signal)

def get_dummy_signal():
    s = np.indices((20,20)).T[np.newaxis][np.zeros((20,20),dtype=int)]
    s = s - np.indices((20,20)).T[:,:,np.newaxis,np.newaxis,:]
    s = 800 - np.sum(s**2,axis=-1)
    return hs.signals.Signal2D(s)

[ericpre]

This looks very promising! :) It is quite a bit to review but I will try to do in the coming weeks.

[sivborg]

I have reviewed some of the functionality recently, and have explored one particular issue with the plotting of a lazy signal after using PolygonROI.
The image for the navigator in the plot is in a lazy signal created by averaging the chunks containing the centre point of the signal. The centre point is the index argument in https://github.com/sivborg/hyperspy/blob/1720d0b1aeaf8822940a738092d58af4aac69241/hyperspy/_signals/lazy.py#L1221
However, in a signal cropped by PolygonROI this centre point can sometimes be cropped out, especially when using a region of multiple polygons. The result is that the navigator becomes blank due to adding NaN values. This also happens if the centre point is close to an edge, leading to some of the values in the chunk becoming NaN.
One way to remedy this is by choosing something else than the centre for index when finding the centre point being NaN in one navigator position (or all). Perhaps using metadata that marks that the signal has been cropped by PolygonROI to activate this case. However, the question is whether it is worth it to change the behaviour of the lazy signal for this. I should remark too that using CircleROI can also result in this, so it is not unique behavior.

[CSSFrancis]

@sivborg would this not be fixed by just having the compute navigator function call nansum? Or are all of the values nan in the middle chunk?

Using the center chunk of the signal for computing the navigator is nice but does introduce some potential inconsistencies/ potential strange behavior.

[sivborg]

@CSSFrancis Using nansum in the compute_navigator function is a possible solution, yes. But this may also happen in a non-lazy signal, so it might be necessary to add it there as well.

Though in my use cases, the centre chunk is often uniformly nan, as I choose multiple polygon at large distances, leaving the middle as NaN. So another way to choose default index could be useful.

I have made a quick working solution, although I am not sure it is worth it to change so much of the existing functionality, potentially introducing some bugs, to fix a visual problem. A user could also fix this themselves by replacing an NaN with zeroes before plotting.

[ericpre]

This look already fairly good and sorry for coming back on this only now.
The API needs to be made consistent with the API of existing widgets - there are some comments in my review on that.
One thing that I find surprising in the current implementation of PolygonROI is that it consist of several polygons and once there are fixed, they can't be changed interactively... I think that this is a significant inconsistency with the existing behaviour of other ROIs. If the aim of this PR is to be able to select/mask areas with several polygons, then this should be done by adding a different functionality which will take care of combining multiple ROIs, but special casing a type of ROIs.

The rest should be small details!

[sivborg]

Thank you for taking a look through! Regarding not being capable of editing existing polygons, an earlier commit actually had that possibility! However it used some "private" features in the matplotlib.widgets.PolygonSelector class, so I removed it to avoid any future problems if the matplotlib.widgets.PolygonSelector) implementation changes. However, by clicking inside an existing polygon, so it is highlighted in red, then pressing delete, you can remove and then redraw existing polygons. It is a bit clunky, but more safe wrt. the matplotlib features. Perhaps a PR in matplotlib could be useful? However, if you think we should restrict to a single polygon then that is also a possibility.

[ericpre]

Why do you need to access private API? In the following example, you can edit the polygon:

import matplotlib.pyplot as plt
from matplotlib.widgets import PolygonSelector

fig, ax = plt.subplots()

selector2 = PolygonSelector(ax, lambda *args: None)

[sivborg]

It is mostly a problem in the case where you have multiple polygons. It is possible to attach multiple PolygonSelector, however using several in one plot can make things very slow and do not work together smoothly, as far as I recall. PolygonROI can change to only implement one polygon at a time, though that would limit its use cases.

[ericpre]

It is mostly a problem in the case where you have multiple polygons. It is possible to attach multiple PolygonSelector, however using several in one plot can make things very slow and do not work together smoothly, as far as I recall. PolygonROI can change to only implement one polygon at a time, though that would limit its use cases.

This is an important point which is not consistent with other ROIs and I don't think that it is a good idea to introduce a different behaviour for this ROI. There is currently no pattern to handle multiple ROI in hyperspy and this is true that in some situations (ROI overlapping) multiple ROI doesn't play together. However, this is issue should be handle separately from the PolygonROI that you are adding here. I don't expect the multiple issue will be very difficult to address but it should be done in a separate PR and for all ROIs or least check that the implementation play well with other ROIs.

[sivborg]

I am working on reimplementing an older commit where there was only one polygon in the ROI, for consistency. I am a bit busy currently but hope to have it done early next month!

[ericpre]

Re-opening because this has been closed automatically by mistake!

[ericpre]

I expected that it would be possible to add vertices manually after creating the polygon, in which case, the number of vertices could be changed once a polygon is completed, but this isn't to be the case - vertices can only be removed, not added. This is a shame and this is something that would be useful in matplotlib.

Can you add example in the gallery? I will try to review in the coming days.

[sivborg]

I expected that it would be possible to add vertices manually after creating the polygon, in which case, the number of vertices could be changed once a polygon is completed, but this isn't to be the case - vertices can only be removed, not added. This is a shame and this is something that would be useful in matplotlib.

Yes, that is true. Although it might not be too difficult to add at some point in the future.

Can you add example in the gallery? I will try to review in the coming days.

I have added an example to use PolygonROI in the examples folder. Although this example is only on signal axes, so perhaps it would be interesting to expand it to show how it can be used in 4D, by slicing either the signal or navigation space.

[ericpre]

I push a few commits to:

• fix and improve documentation - readthedocs is still failing which is surprising
• start to add some tests - more are needed to increase the coverage in the roi.py module
• improve some code.

When using multiple selectors, they all moved together and this behaviour should be fixed in matplotlib/matplotlib#29027.

What is left is to increase the test coverage as highlighted by the codecov warning in the "files changes" tab.

[sivborg]

I have now added some further test coverage, with only a couple NotImplementedErrors to remain. Now, however, the tests seem to fail due to some deprecated functionality in the prettytable package.
I think also there might have snuck a couple bugs into the polygon building, so I will see if I can get a handle on them.

[sivborg]

I should have now fixed the bug in the polygon construction. It was created since the update of the ROI was moved from the PolygonSelector's onselect callback to the Matplotlib _onmove callback. The former is only called when the polygon is finished constructing, while the latter is called continuously. So the way to fix it was to check more thoroughly if the new vertices are valid and not set them until the polygon is constructed.

[ericpre]

Thank you @sivborg, it wasn't an easy task to implement this type of ROI!

If there are things that need tweaking, this can be done in follow up PRs.

[sivborg]

And thank you for the feedback and help, @ericpre !
While a challenge, it came with both valuable experiences and newfound skills :) No doubt I can use this competence in Git and software development in the future!
And I do think there are some tweaks to be had - but we will see what is most immediate.
Until next time!