Allow enforcing persistenly circular ROI in draw tool #376

dhomeier · 2023-07-18T17:15:30Z

Description

Substitute for #370 that does not alter the default behaviour (of drawing an ellipse when x and y radius are > 2 % different), to avoid issues as #370 (comment).

Instead returning a CircularROI is only enforced by setting a kwarg strict_circle=True, which is made available as default in BqplotTrueCircleMode and 'bqplot:truecircle'.

@camipacifici, @pllim I checked with the tests adapted from #370, but could not test the Jdaviz examples, since I cannot reproduce the original issue with either workflow at the moment. But replacing BqplotCircleMode with BqplotTrueCircleMode in Jdaviz tools with this PR should in hopefully fix them.

@astrofrog yet to decide if this is to be exposed by a public kwarg; also not sure if we still need the additional interface (and test) through 'bqplot:ellipse' with this approach.

codecov · 2023-07-18T17:20:23Z

Codecov Report

Patch coverage: 81.63% and project coverage change: +0.29% 🎉

Comparison is base (f0270ab) 86.65% compared to head (449592f) 86.95%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #376      +/-   ##
==========================================
+ Coverage   86.65%   86.95%   +0.29%     
==========================================
  Files          89       89              
  Lines        4940     4984      +44     
==========================================
+ Hits         4281     4334      +53     
+ Misses        659      650       -9

Files Changed	Coverage Δ
glue_jupyter/bqplot/common/tools.py	`65.50% <59.09%> (+3.43%)`	⬆️
glue_jupyter/bqplot/image/viewer.py	`87.50% <100.00%> (ø)`
glue_jupyter/bqplot/tests/test_bqplot.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

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

camipacifici · 2023-07-18T19:39:06Z

Hi @dhomeier, I tried testing your solution here, but I am encountering an error when opening jdaviz in stand alone mode:
Traceback (most recent call last):
File "/Users/cpacifici/opt/miniconda3/envs/prderek/bin/jdaviz", line 5, in
from jdaviz.cli import _main
File "/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/jdaviz/init.py", line 16, in
from jdaviz.configs.specviz import Specviz # noqa: F401
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/jdaviz/configs/init.py", line 1, in
from .cubeviz import * # noqa
^^^^^^^^^^^^^^^^^^^^^^
File "/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/jdaviz/configs/cubeviz/init.py", line 1, in
from .plugins import * # noqa
^^^^^^^^^^^^^^^^^^^^^^
File "/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/jdaviz/configs/cubeviz/plugins/init.py", line 1, in
from .tools import * # noqa
^^^^^^^^^^^^^^^^^^^^
File "/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/jdaviz/configs/cubeviz/plugins/tools.py", line 7, in
from jdaviz.configs.imviz.plugins.tools import _MatchedZoomMixin
File "/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/jdaviz/configs/imviz/init.py", line 1, in
from .plugins import * # noqa
^^^^^^^^^^^^^^^^^^^^^^
File "/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/jdaviz/configs/imviz/plugins/init.py", line 2, in
from .viewers import * # noqa
^^^^^^^^^^^^^^^^^^^^^^
File "/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/jdaviz/configs/imviz/plugins/viewers.py", line 15, in
from jdaviz.configs.default.plugins.viewers import JdavizViewerMixin
File "/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/jdaviz/configs/default/init.py", line 1, in
from .plugins import * # noqa
^^^^^^^^^^^^^^^^^^^^^^
File "/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/jdaviz/configs/default/plugins/init.py", line 1, in
from .viewers import * # noqa
^^^^^^^^^^^^^^^^^^^^^^
File "/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/jdaviz/configs/default/plugins/viewers.py", line 5, in
from glue_jupyter.bqplot.scatter.layer_artist import BqplotScatterLayerState
ImportError: cannot import name 'BqplotScatterLayerState' from 'glue_jupyter.bqplot.scatter.layer_artist' (/Users/cpacifici/opt/miniconda3/envs/prderek/lib/python3.11/site-packages/glue_jupyter/bqplot/scatter/layer_artist.py)

I think I have the exact workflow where I see the problem. It happens when I pop out the viewer from the notebook. When popped out, the x,y aspect ratio is not retained (spacetelescope/jdaviz#2187) and that is why a circular subset becomes an ellipse. We need to fix the aspect ratio bug, but I still think that a circle should be a circle regardless. Let me know if I need to do something different to test this PR. I can send a video of how I trigger the problem if that can be helpful. Thank you!

dhomeier · 2023-07-18T21:03:27Z

I think I have the exact workflow where I see the problem. It happens when I pop out the viewer from the notebook. When popped out, the x,y aspect ratio is not retained (spacetelescope/jdaviz#2187) and that is why a circular subset becomes an ellipse. We need to fix the aspect ratio bug, but I still think that a circle should be a circle regardless. Let me know if I need to do something different to test this PR. I can send a video of how I trigger the problem if that can be helpful. Thank you!

Thanks for the details, I had been missing that you were creating a new window for the viewer here as well!
I could now reproduce the EllipticalROI created originally, and can confirm that with this PR I get a CircularROI when using 'bqplot:truecircle' in the viewer. This also remains so when moving the created subset, but still changes to a EllipticalROI when I am resizing it later. This probably still needs fixing, have to check if it's an issue within the glue-jupyter tools as well.

Your failure in launching jdaviz in the first place is due to the missing fix for BqplotScatterLayerState which I pushed separately to #375, since it is a more immediate bugfix. You'll have to pull that in addition; if you have problems with that I might push the commit here as well while this is still in draft stage.

dhomeier · 2023-07-19T17:45:29Z

The first screenshot shows region properly returning the CircularROI after dragging and moving it, while in the second after resizing it has switched to EllipticalROI. My take is that the resize handle still creates a (new) plain BqplotCircleMode, but I could not find this defined anywhere in Jdaviz – is this a default operation from glue-jupyter?

pllim · 2023-07-24T20:10:15Z

@dhomeier , this line needs to be changed in Imviz to use your new feature:

https://github.com/spacetelescope/jdaviz/blob/9f12e594d54a4fb3fdbcadf26687e34b4297a69c/jdaviz/configs/imviz/plugins/viewers.py#L27

@camipacifici , your error is from #363 (review) and looks like it is being fixed over at #375 .

dhomeier · 2023-08-01T14:22:47Z

@camipacifici , your error is from #363 (review) and looks like it is being fixed over at #375 .

Yes it should. I'll probably have the final version raise a DeprecationWarning, so this needs to be checked for your test suite – I was unable to run Jdaviz tests so far because I could not get it to ignore another Deprecation coming in from echo.

pllim · 2023-08-01T15:13:05Z

because I could not get it to ignore another Deprecation coming in from echo

To get past that, you can temporarily ignore that echo warning in Jdaviz pyproject.toml in the pytest filterwarnings section. We are not seeing this in CI, so maybe you can also try to upgrade echo or something.

dhomeier · 2023-08-01T20:55:32Z

To get past that, you can temporarily ignore that echo warning in Jdaviz pyproject.toml in the pytest filterwarnings section. We are not seeing this in CI, so maybe you can also try to upgrade echo or something.

Yes, putting it there rather than playing around in setup.cfg got around it (maybe the filterwarnings there should be removed?). AFAICS you are pinning to echo==0.5, but I am getting this and another pkg_resources one both with 0.5 and 0.8.0 – it probably should be fixed by glue-viz/echo#36, which is not merged yet.
But anyway after running next into the Jupyter is migrating its paths to use standard platformdirs warning, my mambaforge python segfaults on running the tests, so I'll leave it up to you to test the updated #375 (non-public, deprecated BqplotScatterLayerState) for now. Can maybe test with my work laptop tomorrow.

pllim · 2023-08-01T21:03:57Z

Re: #376 (comment)

Are you sure you are not working off an old copy of Jdaviz code? We don't pin to echo==0.5 on main. See https://github.com/spacetelescope/jdaviz/blob/c3168d4dd562ba143ca86ed41178177d685fccf7/pyproject.toml#L17

But the duplication in setup.cfg is unexpected and I fear it might be a botched rebase on very old PR that got merged. I will open issue about that on Jdaviz (spacetelescope/jdaviz#2331). Good catch!

dhomeier · 2023-08-01T21:15:31Z

Are you sure you are not working off an old copy of Jdaviz code? We don't pin to echo==0.5 on main.

No, was checking what you were running in your CI, but misread echo>=0.5.0 for echo==0.5 – it was installing 0.8.0 indeed.
Should get that fix in for echo anyway.

from glue-viz/glue-jupyter#376

pllim · 2023-08-01T22:23:37Z

@dhomeier , I created spacetelescope/jdaviz#2332 to test this downstream. Seems to work as expected. Thanks!

pllim · 2023-08-02T14:29:29Z

@dhomeier , can you please rebase to pick up #375 here? Thanks!

dhomeier · 2023-08-02T14:35:57Z

Already did; something must have stalled when pushing.

pllim · 2023-08-02T14:45:53Z

The third commit after rebase looks unrelated.

pllim · 2023-08-02T15:49:15Z

Re: #376 (comment)

What does "resizing" mean? How and what do you "resize"?

dhomeier · 2023-08-02T15:55:33Z

The third commit after rebase looks unrelated.

Yes, but necessary for BqplotScatterLayerState not to raise a UserWarning, which would break Jdaviz tests.
Can put this in its own follow-up PR if this one takes too long.

dhomeier · 2023-08-02T15:58:02Z

Re: #376 (comment)

What does "resizing" mean? How and what do you "resize"?

In the image example the (only) Subset0, originally linked to a CircularROI. When placing the mouse over the perimeter rather than inside the circle, so resize mode is activated instead of moving, once the radius is changed it becomes the EllipticalROI in the second image (becoming again a circle in display coordinates, but elliptical in data coordinates).

pllim · 2023-08-02T15:58:11Z

@dhomeier I think the DeprecationWarning fix should be its own PR and merged immediately. Thanks!

dhomeier · 2023-08-02T16:26:38Z

Will rebase again once #379 is approved and merged.

dhomeier · 2023-08-09T14:15:34Z

Should we maybe add a private attribute on CircularROI to indicate whether it was created by the strict mode?

I have now implemented your idea from the SME meeting defining this as a class, so that initialising from a TrueCircularROI will also set self._strict_circle = True by default (for some reason special-casing this in

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

Line 585 in f0270ab

if isinstance(roi, (EllipticalROI, CircularROI)):

breaks the drag-and-move mode when first initialising the brush selector, so I moved the functionality to update_from_roi instead).
Having strict_circle also as a kwarg complicates things a bit in this version, and has become pretty much redundant anyway, so perhaps it should just be removed?
I have verified resizing in the Voila app and ran Jdaviz tests locally, but did not check against the non-Imviz case as in
#370 (comment)

pllim · 2023-08-09T16:34:38Z

glue_jupyter/bqplot/common/tools.py

+
+        super().__init__(viewer, **kwargs)
+
+        if kwargs.pop('strict_circle', True) is False:


Yeah, I agree this is redundant. What do you think, @astrofrog ?

dhomeier · 2023-08-10T14:00:22Z

We don't have any tests directly calling the Bqplot*Mode interfaces directly, should we?
Otherwise if Jdaviz confirms this fixes the issue fully, it should be ready to go now.

pllim · 2023-08-10T14:07:25Z

glue_jupyter/bqplot/tests/test_bqplot.py

@@ -5,6 +5,7 @@
 from nbconvert.preprocessors import ExecutePreprocessor
 from glue.core import Data
 from glue.core.roi import EllipticalROI
+from ..common.tools import TrueCircularROI


Do we still need relative import in Python 3?

Dunno, is it discouraged now?

pllim · 2023-08-10T14:08:27Z

Oh, you mean test for mode, not the tool, nevermind, please ignore my comment about test. I don't need new tests beyond what you added here.

dhomeier · 2023-08-10T21:40:33Z

I did pull in your test and added another one for the TrueCircle version; should have listed you as co-author.

pllim · 2023-08-14T18:54:06Z

Thanks, @dhomeier ! Please let us know when this feature is released, so I can revisit the downstream PR.

dhomeier · 2023-08-15T12:51:13Z

@astrofrog how about a bugfix release once #384 and #385 are in, and perhaps see if we can get #383 finished within a day or so?

astrofrog · 2023-08-15T13:42:53Z

Sounds good - although maybe a minor version rather than bugfix since this is a new feature?

dhomeier · 2023-08-15T13:49:31Z

Seemed it was considered a bug 😁

But yes, it is new functionality, and also a new tool in the standard notebook – maybe I should try to find an alternative icon for that.

pllim · 2023-08-15T15:52:37Z

although maybe a minor version rather than bugfix since this is a new feature?

Yeah, strictly speaking, it is a new feature though it acts like a bug in Jdaviz (mainly due to user confusion).

dhomeier · 2023-08-16T13:35:38Z

@astrofrog, in view of the screenshot above (which is from #383) I am wondering which of the new subset tools should remain enabled by default. I had added them just because I did not know how to make them available in the tests otherwise, but I think the "circular annulus" would be genuinely useful. But "circular" and "strictly circular" may be more confusing in the present form; perhaps renaming it again to "persistent circular" would be a bit clearer, but I can't think of a good icon to symbolise that (maybe changing the non-persistent one to something combining circular and elliptic shapes). Or is there a way to add this to tools just for the tests?

astrofrog · 2023-08-17T10:59:31Z

The annulus one would only make sense to include by default if we had a way to edit the inner radius - which I don't think we do here? (but there is in jdaviz)

You can add some of these just for tests by making a test viewer subclass where you edit the tools attribute - I think that's probably the easiest way.

For now I don't think the strict circular mode should be enabled by default until we've had a bit more time to think about it - for instance it will only really be intuitive in the image viewer if aspect is equal, but if aspect is auto it will be a bit confusing. For now I would prefer to not expose this except in jdaviz.

So for now I would leave out the annulus and strict circle tools from the defaults.

dhomeier · 2023-08-17T21:22:20Z

The annulus one would only make sense to include by default if we had a way to edit the inner radius - which I don't think we do here? (but there is in jdaviz)

Isn't there a way to access the subset through viewer.layers? But that's certainly not a viable interface for the standard notebook setup.

You can add some of these just for tests by making a test viewer subclass where you edit the tools attribute - I think that's probably the easiest way.

I've tried to do that, but could not figure out yet how the viewer.toolbar.tools dictionary is produced from the tools list in viewer.py, and correspondingly what's the correct way to modify it – I suppose this is done somewhere inside bqplot?

Carifio24 · 2023-08-17T23:09:33Z

@dhomeier If it helps, the viewer.toolbar.tools dictionary is created via the add_tool calls here. So for a subclass all you should need to is define the tools = ["some_tool_id_1", "some_tool_id_2", ...] attribute on the viewer class and the toolbar will get populated with the corresponding tools (plus base class tools, unless you set inherit_tools = False).

dhomeier · 2023-08-18T13:56:00Z

Thanks @Carifio24 , exactly the bit of info I needed! In ca50995 tests are working with just calling add_tool on the viewer instance, so creating a new subclass should not even be required.

from glue-viz/glue-jupyter#376

* Use truecircle tool from glue-viz/glue-jupyter#376 * MNT: Bump glue-jupyter minversion that can use this new tool * Add change log * Fix line too long * Fix change log verbiage Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

fix event handling for choosing config from launcher filepath fallback when object from config-detection fails to load fix: support for notebook 7 (spacetelescope#2420) Notebook 7 requires a different way to detect if the app runs in a notebook context. Fix explanation in markdown cell. DOC: Footprints plugin API docs (spacetelescope#2426) * build footprints API docs * add code snippet in plugin docs * fix syntax in API docs --------- Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> prevent overwriting user-API methods (spacetelescope#2425) fix default color for overlay when traitlet set by user * for the FIRST overlay, the user could have set the color traitlet before the internal overlay object is created (which occurs as soon as the plugin is first shown). In that case, we want to respect the user choice of color. similar fixes for any other traitlet when plugin inactive regression test change log entry fix API import of single region and overwriting existing entry * along with regression testing TST: Also pull in scikit-image dev to get rid of incompatibility with numpy Use new "true circle" tool from glue-jupyter (spacetelescope#2332) * Use truecircle tool from glue-viz/glue-jupyter#376 * MNT: Bump glue-jupyter minversion that can use this new tool * Add change log * Fix line too long * Fix change log verbiage Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> Enable multiselect in subset plugin for recentering (spacetelescope#2430) * Enable multiselect in subset plugin for recentering * Fix bug with default text * Remove unused import * Fix bug in aperature photometry * Address review comments * Remove commented out code * Add initial tests * Fix bug when exiting multiselect and switching subsets * Add documentation and update CHANGES file * Remove print statement * Add comment about recentering taking multiple iterations to docs * fix chip styling in subset dropdown * only set selected_has_subregions if exists * only show multiselect toggle in imviz * Apply suggestions from code review Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> Debug standalone build (spacetelescope#2441) * Log jdaviz output for debugging * Run workflow on debugging branch * See if collecting data from jupyter-client will fix this, otherwise might need to downgrade * Remove debugging stuff MNT: Bump actions/checkout to v4 Enable workflow_dispatch for standalone because it cannot be enabled outside of main apparently [ci skip] [rtd skip] EXP: Update workflow versions Fix Mosviz slit overlay (spacetelescope#2434) * Fix Mosviz slit overlay by using polygon instead of forcing a rectangle without angle info. Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com> * Off by one error in predicted PR num --------- Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com> API access (hidden) to disable cubeviz movie UI (spacetelescope#2440) * (hidden) API access to disable cubeviz movie UI * default movie_enabled based on new app-setting server_is_remote * test coverage TST: Ignore ASDF warning about gwcs-1.0.0 schema because gwcs dev and/or asdf-wcs-schemas 0.2.0 triggers the warning because the schema is no longer supported.

dhomeier added bug Something isn't working bqplot-viewers labels Jul 18, 2023

dhomeier requested a review from astrofrog July 18, 2023 17:15

dhomeier added the ui label Jul 19, 2023

pllim mentioned this pull request Aug 1, 2023

Remove setup.cfg that was accidentally reintroduced spacetelescope/jdaviz#2331

Closed

pllim added a commit to pllim/jdaviz that referenced this pull request Aug 1, 2023

Use truecircle tool

078e84b

from glue-viz/glue-jupyter#376

This was referenced Aug 1, 2023

Use new "true circle" tool from glue-jupyter spacetelescope/jdaviz#2332

Merged

Circle draw tool must produce circle #370

Closed

pllim approved these changes Aug 1, 2023

View reviewed changes

dhomeier force-pushed the circularisation-of-circle branch from 4c69cd6 to 7ccb5a4 Compare August 2, 2023 14:34

Add strict_circle option and BqplotTrueCircleMode class

ca29542

Create TrueCircularROI

06c7ea7

pllim reviewed Aug 9, 2023

View reviewed changes

Remove strict_circle kwarg

449592f

This comment was marked as outdated.

Sign in to view

pllim reviewed Aug 10, 2023

View reviewed changes

dhomeier merged commit 0be614f into glue-viz:main Aug 12, 2023

dhomeier deleted the circularisation-of-circle branch August 12, 2023 12:52

dhomeier mentioned this pull request Aug 12, 2023

Prevent destruction of CircularAnnulusROI by resizing below inner_radius #383

Merged

astrofrog added the enhancement New feature or request label Aug 15, 2023

dhomeier removed the bug Something isn't working label Aug 15, 2023

dhomeier changed the title ~~Allow enforcing exactly circular ROI in draw tool~~ Allow enforcing persistenly circular ROI in draw tool Aug 15, 2023

pllim added a commit to pllim/jdaviz that referenced this pull request Sep 6, 2023

Use truecircle tool

1f2f3d2

from glue-viz/glue-jupyter#376


		super().__init__(viewer, **kwargs)

		if kwargs.pop('strict_circle', True) is False:

Allow enforcing persistenly circular ROI in draw tool #376

Allow enforcing persistenly circular ROI in draw tool #376

Conversation

dhomeier commented Jul 18, 2023 • edited Loading

Description

codecov bot commented Jul 18, 2023 • edited Loading

Codecov Report

camipacifici commented Jul 18, 2023

dhomeier commented Jul 18, 2023 • edited Loading

dhomeier commented Jul 19, 2023

pllim commented Jul 24, 2023

dhomeier commented Aug 1, 2023

pllim commented Aug 1, 2023

dhomeier commented Aug 1, 2023

pllim commented Aug 1, 2023 • edited Loading

dhomeier commented Aug 1, 2023

pllim commented Aug 1, 2023

pllim commented Aug 2, 2023

dhomeier commented Aug 2, 2023

pllim commented Aug 2, 2023

pllim commented Aug 2, 2023

dhomeier commented Aug 2, 2023

dhomeier commented Aug 2, 2023 • edited Loading

pllim commented Aug 2, 2023

dhomeier commented Aug 2, 2023

dhomeier commented Aug 9, 2023

pllim Aug 9, 2023

Choose a reason for hiding this comment

astrofrog Aug 9, 2023

Choose a reason for hiding this comment

dhomeier commented Aug 10, 2023

This comment was marked as outdated.

pllim Aug 10, 2023

Choose a reason for hiding this comment

dhomeier Aug 10, 2023

Choose a reason for hiding this comment

pllim commented Aug 10, 2023

dhomeier commented Aug 10, 2023

pllim commented Aug 14, 2023

dhomeier commented Aug 15, 2023

astrofrog commented Aug 15, 2023

dhomeier commented Aug 15, 2023

pllim commented Aug 15, 2023

dhomeier commented Aug 16, 2023

astrofrog commented Aug 17, 2023 • edited Loading

dhomeier commented Aug 17, 2023

Carifio24 commented Aug 17, 2023

dhomeier commented Aug 18, 2023

dhomeier commented Jul 18, 2023 •

edited

Loading

codecov bot commented Jul 18, 2023 •

edited

Loading

dhomeier commented Jul 18, 2023 •

edited

Loading

pllim commented Aug 1, 2023 •

edited

Loading

dhomeier commented Aug 2, 2023 •

edited

Loading

astrofrog commented Aug 17, 2023 •

edited

Loading