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.

Sign up for GitHub

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

Jump to bottom

Add __getitem__ method to ROIs #2288

Merged
merged 14 commits into from Dec 9, 2019
Merged

Add __getitem__ method to ROIs #2288

merged 14 commits into from Dec 9, 2019

Conversation

francisco-dlp
Copy link
Member

@francisco-dlp francisco-dlp commented Dec 4, 2019 •
edited

Description of the change

Add __getitem__ method to the following ROIs:

  • Point1DROI
  • Point2DROI
  • SpanROI
  • RectangularROI

In this way, they can be used in place of tuples.

Progress of the PR

  • Change implemented (can be split into several points),
  • update docstring (if appropriate),
  • update user guide (if appropriate),
  • add tests,
  • ready for review.

Minimal example of the bug fix or the new feature

>>> import hyperspy.api as hs
>>> import numpy as np
>>> im = hs.signals.Signal2D(np.random.random((10,30,30))
>>> roi = hs.roi.RectangularROI(left=2, right=10, top=0, bottom=5))
>>> tuple(roi)
(2.0, 10.0, 0.0, 5.0)
>>> im.align2D(roi=roi)

@francisco-dlp francisco-dlp changed the title Align with ro is Add __getitem__ method to some ROIs Dec 4, 2019
@ericpre
Copy link
Member

ericpre commented Dec 6, 2019

This is very nice. A few comments/questions:

  • Any reason not to do it for all ROIs?
  • Since it makes signal_range_from_roi not necessary anymore, should it be deprecated?
  • I guess a couple of docstring could be updated to mention ROI can be passed to argument like, signal_range or roi.
  • Related to previous point, make this kind of argument (signal_range, roi, etc.) consistent across different method in which case we may do this in Release_next_major to avoid introducing more deprecation...

How far do you want to go in this PR?

@francisco-dlp
Copy link
Member Author

francisco-dlp commented Dec 6, 2019 •
edited

  • Any reason not to do it for all ROIs?

Just that it is not straightforwardly useful to get the (x, y, radious, innerradious) values of the CircleROI or the (x1, y1, x2, y2) coordinates of Line2DROI in tuple format. Do you think that it is worth adding it nevertheless for consistency?

  • Since it makes signal_range_from_roi not necessary anymore, should it be deprecated?

Thanks, done!

  • I guess a couple of docstring could be updated to mention ROI can be passed to argument like, signal_range or roi.

Done.

Related to previous point, make this kind of argument (signal_range, roi, etc.) consistent across different method in which case we may do this in Release_next_major to avoid introducing more deprecation...

Yes, let's do that (where reasonable) in 2.0. There is also an inconsistency across HyperSpy in the order of "left", "right", "top", bottom" as different functions use different conventions. Something for 2.0 I think.

@francisco-dlp
Copy link
Member Author

After thinking about it I have added the feature for all ROIs, and I have taken a simpler, faster approach for the implementation.

ericpre
ericpre reviewed Dec 7, 2019
View reviewed changes
Copy link
Member

@ericpre ericpre left a comment

Choose a reason for hiding this comment

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

Looks good to me. I added a few other comments.

hyperspy/models/model1d.py
@@ -463,7 +462,7 @@ def set_signal_range(self, x1=None, x2=None):

"""
try:
x1, x2 = signal_range_from_roi(x1)
x1, x2 = x1
except TypeError:
Copy link
Member

Choose a reason for hiding this comment

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

Should the error below become a ValueError in case the length of the tuple doesn't match?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure about it: I think that if the length of the tuple doesn't match we want to raise the exception because the only valid inputs are a matching ROI or a number.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree. Actually, it looks to me that it is not possible to get a TypeError with/without this PR? If so, shall we removed it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that it is fine as is: you can get a TypeError by passing a number as argument.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, indeed!

hyperspy/models/model1d.py
@@ -494,7 +493,7 @@ def remove_signal_range(self, x1=None, x2=None):

"""
try:
x1, x2 = signal_range_from_roi(x1)
x1, x2 = x1
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

hyperspy/models/model1d.py
@@ -529,7 +528,7 @@ def add_signal_range(self, x1=None, x2=None):

"""
try:
x1, x2 = signal_range_from_roi(x1)
x1, x2 = x1
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

hyperspy/roi.py Outdated
@@ -979,6 +1038,7 @@ def __init__(self, x1, y1, x2, y2, linewidth=0):
super(Line2DROI, self).__init__()
self.x1, self.y1, self.x2, self.y2 = x1, y1, x2, y2
self.linewidth = linewidth
self._tuple = (self.x1, self.y1, self.x2, self.y2)
Copy link
Member

Choose a reason for hiding this comment

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

linewidth?

@francisco-dlp
Copy link
Member Author

Thanks for the review. It should be ready for another iteration.

@francisco-dlp francisco-dlp changed the title Add __getitem__ method to some ROIs Add __getitem__ method to ROIs Dec 9, 2019
ericpre
ericpre reviewed Dec 9, 2019
View reviewed changes
hyperspy/roi.py Outdated
@@ -980,6 +1069,12 @@ def __init__(self, x1, y1, x2, y2, linewidth=0):
self.x1, self.y1, self.x2, self.y2 = x1, y1, x2, y2
self.linewidth = linewidth

def __getitem__(self, *args, **kwargs):
_tuple = (self.x1, self.y1, self.x2, self.y2, self.linewidth)
_tuple = (self.x1, self.y1, self.x2, self.y2)
Copy link
Member

Choose a reason for hiding this comment

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

I guess this line should be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thanks. It turns out that I forgot to add tests for Circle and Line2D. After adding them in the last commit I found 1 more bug of course...

@ericpre ericpre merged commit f35970d into hyperspy:RELEASE_next_minor Dec 9, 2019
@ericpre ericpre added this to the v1.6 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericpre ericpre ericpre left review comments

Assignees
No one assigned
Labels
release highlight type: enhancement
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@francisco-dlp @ericpre