Default values for ROIs

[thomasaarholt]

TLDR:

This lets one call hs.roi.Line2DROI() without specifying coordinates, defaulting to sensible values when called interactively on a signal.

Description of the change

It is cumbersome to have to manually specify the coordinates of a ROI - especially when you just quickly want a ROI on an image to extract a certain part of it with roi.interactive().

This PR is an attempt to allow None (or really, the Undefined traits type) to be an allowed value for ROIs so that they can be properly instantiated at a later time.

I have:

• chosen sensible values as default (the central half of an axis - 1/4 to 3/4). For this I introduced a new helper function get_central_half_limits_of_axis(ax) in hyperspy.utils.axes (new file) that returns indices of the central half of an axes using Support for relative slicing #2386.
• introduced two new methods:
  1. roi.parameters: property that returns the parameter names and values as a dict
  2. roi._set_default_values(): sets the appropriate values when using it interactively with un-set values.
• reduced code repetition by moving methods to parent classes where logical (is_valid, __getitem__ and __repr__)
• updated the __repr__ of ROIs to use f-strings and the :G formatter where possible.

Progress of the PR

• Working implementation for all ROIs
• Need to change the __repr__ of rois to take into account the Undefined trait
• update user guide (if appropriate),
• add tests,
• ready for review.

Minimal example of the bug fix or the new feature

%matplotlib widget
import hyperspy.api as hs

s2 = hs.datasets.artificial_data.get_atomic_resolution_tem_signal2d().isig[:31, :31]
s1 = s2.isig[0,:]
# 1D
pr = hs.roi.Point1DROI()
sr = hs.roi.SpanROI()

roi1 = [pr, sr]

# 2D
cr = hs.roi.CircleROI()
rr = hs.roi.RectangularROI()
lr = hs.roi.Line2DROI()
p2r = hs.roi.Point2DROI()

roi2 = [cr, rr, lr, p2r]

colors = ['blue', 'green', 'red', 'black']
s1.plot()
for i, roi in enumerate(roi1):
    roi.interactive(s1, color = colors[i])


s2.plot()
for i, roi in enumerate(roi2):
    roi.interactive(s2, color = colors[i])
    

[ericpre]

@thomasaarholt, what is the status of this PR? The relative slicing that you added in #2386 could be used here to set the default value?

[thomasaarholt]

Yes! The lack of any comments here made me think that this PR perhaps wasn't desirable, but then again the post is a block of text where I never asked for feedback.

I'm currently deeply involved with a different programming project, but this change should be small enough so I'll try to get it done soon.

[ericpre]

The lack of any comments here made me think that this PR perhaps wasn't desirable, but then again the post is a block of text where I never asked for feedback.

Indeed... actually, I commented on this PR in #2343 (comment).

[codecov]

Codecov Report

Merging #2341 (320371e) into RELEASE_next_minor (1ff7468) will increase coverage by 0.04%.
The diff coverage is 96.59%.

@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #2341      +/-   ##
======================================================
+ Coverage               76.67%   76.71%   +0.04%     
======================================================
  Files                     203      203              
  Lines                   30133    30123      -10     
  Branches                 6566     6565       -1     
======================================================
+ Hits                    23103    23108       +5     
+ Misses                   5210     5199      -11     
+ Partials                 1820     1816       -4     

Impacted Files	Coverage Δ
hyperspy/roi.py	79.61% <96.59%> (+2.67%)	⬆️
hyperspy/_components/skew_normal.py	88.77% <0.00%> (-0.95%)	⬇️
hyperspy/_components/lorentzian.py	97.05% <0.00%> (-0.35%)	⬇️
hyperspy/_components/gaussian.py	97.10% <0.00%> (-0.27%)	⬇️
hyperspy/io_plugins/protochips.py	95.34% <0.00%> (-0.08%)	⬇️
hyperspy/signal.py	75.64% <0.00%> (-0.05%)	⬇️
hyperspy/io_plugins/image.py	83.33% <0.00%> (ø)
hyperspy/drawing/_widgets/range.py	47.08% <0.00%> (+0.75%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ff7468...320371e. Read the comment docs.

[thomasaarholt]

There is one "breaking" change in this:
rect = RectangularROI(...) takes arguments as left, top, right, bottom, but when indexed afterwards as rect[i], it would return it in a different order -> left, right, top, bottom for i = (0,1,2,3). The n
As an example:

>>> rect = hs.roi.RectangularROI(2, 2, 5, 5)
>>> rect
RectangularROI(left=2, top=2, right=5, bottom=5)
>>> rect[2]
2.0

Now it is indexed in the same order.
I'd like to interpret this part of this PR as a bugfix rather than a breaking change. I'm happy to submit it as a different PR if needed.

[ericpre]

I'd like to interpret this part of this PR as a bugfix rather than a breaking change. I'm happy to submit it as a different PR if needed.

Agreed and I think we need to make sure that we get it right... because it can be quite disruptive for users... Definitely better figuring this out in a separate PR.

[thomasaarholt]

I've manually checked that roi.gui() works when the values are undefined.

[ericpre]

See comments to make it more simple.

[thomasaarholt]

Thanks for the comments. I ended up keeping _attributes_have_undefined and _raise_error_if_undefined_attrs, since I ended up using the former a lot today (they are now called _parameters_have_undefined and _raise_error_if_undefined_para).

We now use parameters and values. The former is a dict, the latter is a tuple (and both are @Property). I toyed with creating a parameters class, in similar vein to what we have with models, but decided against it.

I also refactored some code (like self.is_valid()) into BaseROI. I think it fits correctly there. Will probably add some more tests when I see how coverage is looking.

[ericpre]

I have provided more details on why we shouldn't add more method which doesn't add any functionality. The code for the ROIs is already quite long and we shall not make unnecessarily longer.

[ericpre]

Thanks for the comments. I ended up keeping _attributes_have_undefined and _raise_error_if_undefined_attrs, since I ended up using the former a lot today (they are now called _parameters_have_undefined and _raise_error_if_undefined_para).

As mentioned in the review, I am not convinced that adding these method is a good approach.

[thomasaarholt]

@ericpre I think everything is now taken into account. Those were good suggestions, it's a lot cleaner now. It's been a while since I've thought about taking list(self) and what that means.

The failing tests are due to numpy 1.20, see #2635.

The change to setting CircleROI's r_inner=0 means that unless the user sets it manually, it cannot be t.Undefined. Could you look over my last commit with a critical eye and see if you agree with the changes?

[ericpre]

Thanks, I find this version much better! My reviews may have sound a bit picky and annoying but I really think, we should keep the ROI classes as tidy as possible.

I think the structure is good now and the suggestions I have added, are mainly about tests and other improvements.

[ericpre]

Would it be possible to finish this PR while it is getting closed to be finished?

[thomasaarholt]

Should be good now.
Edit: Fixing now

Ok, please take a look

[ericpre]

Great, thanks!