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

BUG: Prevent some spatial ROI from getting too small #416

Closed
wants to merge 1 commit into from
Closed

BUG: Prevent some spatial ROI from getting too small #416

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 31 additions & 2 deletions glue_jupyter/bqplot/common/tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,21 @@
x = self.interact.selected_x
y = self.interact.selected_y
Comment on lines 153 to 154
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
x = self.interact.selected_x
y = self.interact.selected_y

set below

theta = None if self._roi is None else self._roi.theta
roi = RectangularROI(xmin=min(x), xmax=max(x), ymin=min(y), ymax=max(y), theta=theta) # noqa: E501
x_min = min(x)
x_max = max(x)
y_min = min(y)
y_max = max(y)
# Avoid drawing invisible shape.
if isinstance(self._roi, RectangularROI):
dx = abs(x_max - x_min)
dy = abs(y_max - y_min)
if dx < 1:
x_min = self._roi.xmin
x_max = self._roi.xmax
if dy < 1:
y_min = self._roi.ymin
y_max = self._roi.ymax

Check warning on line 169 in glue_jupyter/bqplot/common/tools.py

View check run for this annotation

Codecov / codecov/patch

glue_jupyter/bqplot/common/tools.py#L162-L169 

Added lines #L162 - L169 were not covered by tests
Comment on lines +156 to +169
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
x_min = min(x)
x_max = max(x)
y_min = min(y)
y_max = max(y)
# Avoid drawing invisible shape.
if isinstance(self._roi, RectangularROI):
dx = abs(x_max - x_min)
dy = abs(y_max - y_min)
if dx < 1:
x_min = self._roi.xmin
x_max = self._roi.xmax
if dy < 1:
y_min = self._roi.ymin
y_max = self._roi.ymax
# Avoid drawing invisible shape.
x_min = min(min(self.interact.selected_x), max(self.interact.selected_x) - 1.0)
x_max = max(max(self.interact.selected_x), min(self.interact.selected_x) + 1.0)
y_min = min(min(self.interact.selected_y), max(self.interact.selected_y) - 1.0)
y_max = max(max(self.interact.selected_y), min(self.interact.selected_y) + 1.0)

Simpler to catch too small sizes already here imo; also the later check for dx < 1 has the disadvantage of abruptly snapping back to the original size, which seems less intuitive to handle.
In principle an offset of 0.5 above should be enough, but I could not reliably keep it from resizing below 1 with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I want it to snap back to original size. The actual bug we encounter is explained in the original post above and repeated here:

selected_x: [34.543987, 35.289345]
selected_y: [18.55497, 17.809614]  <--- Suspicious input!!!
xc=34.92, yc=18.18, r=0.37    <--- Problem!!!

It happens when user drags and move an existing shape, not trying to resize it. So snapping to original size is the desired behavior.

If you know why the input is weird in the first place and able to fix that, even better. But I could not figure it out.

Copy link
Contributor

@dhomeier dhomeier Dec 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It happens when user drags and move an existing shape, not trying to resize it. So snapping to original size is the desired behavior.

Ah, I missed that part. But if the selector is in move mode, it should never change the dx/r at all – this sounds rather like a bug in bqplot.
I could not reproduce this with either rectangle or circle, however fast I tried to drag them around – do you have any more details, or a specific dataset with which to reproduce? Otherwise, how can we be sure that a given size indicates this bug showed up – as @astrofrog noted, even very small ROI sizes can be legit?

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 cannot consistently reproduce this bug. It showed up in Imviz like this (after many tries):

  1. Use https://github.com/spacetelescope/jdaviz/blob/main/notebooks/ImvizExample.ipynb
  2. Load the data.
  3. Draw a circle subset on image viewer. I didn't find it matter to finalize it or not; you can try it both ways.
  4. Grab the drawn subset and keeps moving it around.

If you print the selected_x and selected_y out from glue-jupyter code here, you will see it eventually when the circle suddenly "disappears" but is actually super tiny.

roi = RectangularROI(xmin=x_min, xmax=x_max, ymin=y_min, ymax=y_max, theta=theta)
self._roi = roi
self.viewer.apply_roi(roi)
if self.finalize_callback is not None:
Expand Down Expand Up @@ -398,6 +412,17 @@
yc = y.mean()
rx = abs(x[1] - x[0])/2
ry = abs(y[1] - y[0])/2
# Avoid drawing invisible shape.
if isinstance(self._roi, CircularROI):
if rx < 1:
rx = self._roi.radius
if ry < 1:
ry = self._roi.radius

Check warning on line 420 in glue_jupyter/bqplot/common/tools.py

View check run for this annotation

Codecov / codecov/patch

glue_jupyter/bqplot/common/tools.py#L417-L420 

Added lines #L417 - L420 were not covered by tests
elif isinstance(self._roi, EllipticalROI):
if rx < 1:
rx = self._roi.radius_x
if ry < 1:
ry = self._roi.radius_y

Check warning on line 425 in glue_jupyter/bqplot/common/tools.py

View check run for this annotation

Codecov / codecov/patch

glue_jupyter/bqplot/common/tools.py#L422-L425 

Added lines #L422 - L425 were not covered by tests
Comment on lines 413 to +425
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
rx = abs(x[1] - x[0])/2
ry = abs(y[1] - y[0])/2
# Avoid drawing invisible shape.
if isinstance(self._roi, CircularROI):
if rx < 1:
rx = self._roi.radius
if ry < 1:
ry = self._roi.radius
elif isinstance(self._roi, EllipticalROI):
if rx < 1:
rx = self._roi.radius_x
if ry < 1:
ry = self._roi.radius_y
# Avoid drawing invisible shape.
rx = max(abs(x[1] - x[0])/2, 1.0)
ry = max(abs(y[1] - y[0])/2, 1.0)

Same as for rectangle

# We use a tolerance of 1e-2 below to match the tolerance set in glue-core
# https://github.com/glue-viz/glue/blob/6b968b352bc5ad68b95ad5e3bb25550782a69ee8/glue/viewers/matplotlib/state.py#L198
if self._strict_circle:
Expand Down Expand Up @@ -519,7 +544,11 @@
rx = abs(x[1] - x[0]) * 0.5
ry = abs(y[1] - y[0]) * 0.5
outer_r = float(rx + ry) * 0.5
if self._roi is None:
# Avoid drawing invisible shape.
if isinstance(self._roi, CircularAnnulusROI) and outer_r < 2:
outer_r = self._roi.outer_radius
inner_r = self._roi.inner_radius

Check warning on line 550 in glue_jupyter/bqplot/common/tools.py

View check run for this annotation

Codecov / codecov/patch

glue_jupyter/bqplot/common/tools.py#L549-L550 

Added lines #L549 - L550 were not covered by tests
elif self._roi is None:
Comment on lines +547 to +551
Copy link
Contributor

@dhomeier dhomeier Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
# Avoid drawing invisible shape.
if isinstance(self._roi, CircularAnnulusROI) and outer_r < 2:
outer_r = self._roi.outer_radius
inner_r = self._roi.inner_radius
elif self._roi is None:
# Avoid drawing invisible shape.
outer_r = max(float(rx + ry) * 0.5, 2.0)

Should really just replace outer_r above the deleted line.
Note that to avoid drawing invisible annuli, this should perhaps also ascertain that inner_r <= outer_r - 1.0 – #383 just made sure the CircularAnnulusROI is not destroyed here, but did not guarantee it remained visible and interactively accessible.

inner_r = outer_r * 0.5 # Hardcoded for now, user can edit later.
else:
# Resizing only changes outer r, avoid having inner_r >= outer_r afterwards.
Expand Down