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

pllim · 2023-12-19T23:06:27Z

Description

In Jdaviz, we run into a problem where user was dragging around an existing ROI and then it suddenly disappeared. User description was incomplete but I think I reproduced the problem using circular ROI on image viewer.

When tracing the ROI properties here:

glue-jupyter/glue_jupyter/bqplot/common/tools.py

Line 385 in b515772

def update_selection(self, *args):

I saw this:

selected_x: [21.875198, 30.624802]
selected_y: [23.72376, 14.974156]
xc=26.25, yc=19.35, r=4.37

selected_x: [19.041868, 27.791471]
selected_y: [13.640821, 22.39043]
xc=23.42, yc=18.02, r=4.37

selected_x: [18.875202, 27.624805]
selected_y: [18.64082, 27.390429]
xc=23.25, yc=23.02, r=4.37

selected_x: [25.3752, 34.1248]
selected_y: [15.47415, 24.223763]
xc=29.75, yc=19.85, r=4.37

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

I think listening to both selected_x and selected_y could be causing some weird race condition when both are being changed rapidly at the same time. I don't know how to prevent that from happening, so this patch is to just not create a shape that we know is too small. I am only patching the shapes that Imviz cares about.

With this patch:

selected_x: [28.19703  32.969635]
selected_y: [28.235262 23.462654]
Before: xc=30.58, yc=25.85, rx=2.39, ry=2.39
After: rx=2.386302947998047, ry=2.3863039016723633

selected_x: [31.083334 32.083332]
selected_y: [22.348959 21.348959]
Before: xc=31.58, yc=21.85, rx=0.50, ry=0.50  <--- Too small
After: rx=2.3863034248352526, ry=2.3863034248352526   <--- But we prevent that from going in

Not sure how to test this in the CI. I don't see any existing test that deal with moving ROI around after creation.

🐱

getting too small

codecov · 2023-12-19T23:10:00Z

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (307a11d) 86.63% compared to head (19e419a) 86.35%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
glue_jupyter/bqplot/common/tools.py	35.71%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #416      +/-   ##
==========================================
- Coverage   86.63%   86.35%   -0.28%     
==========================================
  Files          89       89              
  Lines        5163     5189      +26     
==========================================
+ Hits         4473     4481       +8     
- Misses        690      708      +18

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

dhomeier

Looks OK in principle, just per the suggestions I think this can be implemented in a simpler and hopefully better to handle way.
The question is what is a too small size really – I had to zoom in very close to even get down to 1 pixel, and still was not able to really make the ROI "vanish" in any form, as e.g. in the Imviz example the subset is still accessible through the plugin menu, and sizes there could still be manually adjusted to make it visible again.

dhomeier · 2023-12-22T23:20:29Z

glue_jupyter/bqplot/common/tools.py

+                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


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.

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.

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?

I cannot consistently reproduce this bug. It showed up in Imviz like this (after many tries):

Use https://github.com/spacetelescope/jdaviz/blob/main/notebooks/ImvizExample.ipynb

Load the data.

Draw a circle subset on image viewer. I didn't find it matter to finalize it or not; you can try it both ways.

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.

dhomeier · 2023-12-22T23:22:38Z

glue_jupyter/bqplot/common/tools.py

                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


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

dhomeier · 2023-12-22T23:25:43Z

glue_jupyter/bqplot/common/tools.py

+                # 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:


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.

dhomeier · 2023-12-23T00:07:33Z

glue_jupyter/bqplot/common/tools.py

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


Suggested change

x = self.interact.selected_x

y = self.interact.selected_y

set below

astrofrog · 2023-12-23T10:23:42Z

Just a quick note that what might be too small in an image viewer may not be in a scatter plot viewer. A scatter plot may have some points all contained in e.g. [1.12221:1.12222]

pllim · 2023-12-23T17:00:02Z

Re: #416 (comment)

We do not use scatter plot like that in Jdaviz (not yet anyway) so I do not grok how the tool is supposed to work there. But why would someone purposefully make a super tiny spatial subset?

kecnry · 2023-12-26T14:26:36Z

I agree that setting a minimum size for all subsets and reverting otherwise can have unintended consequences elsewhere and is probably just a bandaid on the actual bug. Even for Imviz, we need to support subsets with sizes smaller than 1 pixel in the case where the reference data has pixels much larger than other linked images.

dhomeier · 2023-12-26T14:39:52Z

I haven't been able to figure out at all how the BrushSelector distinguishes between move and resize operations – the former are triggered whenever a brushing interaction is started inside the region rather than on its edges, and in that case all of interact.selected should only change in synchronisation (radius etc. unchanged). But where is that info about the type of interaction stored?

I think listening to both selected_x and selected_y could be causing some weird race condition when both are being changed rapidly at the same time.

Such a race condition would also have to occur entirely inside the bqplot level since update_selection is not doing anything before the moving operation is finalised and self.interact.brushing set to False, right?
Maybe one could change the tool to only self.interact.observe(self.on_selection_change, "selected"),
I think this should still cover all changes in x or y and might perhaps reduce the listening traffic a bit?

pllim · 2023-12-26T21:13:33Z

But where is that info about the type of interaction stored?

I couldn't figure it out either. I have a feeling that the info has been lost by the time that method to update shape gets triggered.

Since this is obviously not going to go forward, no point keeping this PR open. I am off this week. Thanks for the prompt feedback!

BUG: Prevent some spatial ROI from

19e419a

getting too small

dhomeier requested changes Dec 22, 2023

View reviewed changes

dhomeier reviewed Dec 23, 2023

View reviewed changes

pllim closed this Dec 26, 2023

pllim deleted the fix-imviz-roi-too-small branch December 26, 2023 21:13

pllim mentioned this pull request Jan 2, 2024

Let spatial shape tool block resizing attempt #418

Open

pllim mentioned this pull request Feb 6, 2024

Set InteractCheckableTool._roi to None on deactivate #420

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pllim commented Dec 19, 2023

codecov bot commented Dec 19, 2023

dhomeier left a comment

dhomeier Dec 22, 2023

pllim Dec 23, 2023

dhomeier Dec 23, 2023 •

edited

Loading

pllim Dec 23, 2023

dhomeier Dec 22, 2023

dhomeier Dec 22, 2023 •

edited

Loading

dhomeier Dec 23, 2023

astrofrog commented Dec 23, 2023

pllim commented Dec 23, 2023

kecnry commented Dec 26, 2023

dhomeier commented Dec 26, 2023

pllim commented Dec 26, 2023

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

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

Conversation

pllim commented Dec 19, 2023

Description

codecov bot commented Dec 19, 2023

Codecov Report

dhomeier left a comment

Choose a reason for hiding this comment

dhomeier Dec 22, 2023

Choose a reason for hiding this comment

pllim Dec 23, 2023

Choose a reason for hiding this comment

dhomeier Dec 23, 2023 • edited Loading

Choose a reason for hiding this comment

pllim Dec 23, 2023

Choose a reason for hiding this comment

dhomeier Dec 22, 2023

Choose a reason for hiding this comment

dhomeier Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

dhomeier Dec 23, 2023

Choose a reason for hiding this comment

astrofrog commented Dec 23, 2023

pllim commented Dec 23, 2023

kecnry commented Dec 26, 2023

dhomeier commented Dec 26, 2023

pllim commented Dec 26, 2023

dhomeier Dec 23, 2023 •

edited

Loading

dhomeier Dec 22, 2023 •

edited

Loading