Aperture live-preview plugin marks #2664

kecnry · 2024-01-18T14:09:40Z

Description

This pull request implements live-preview plugin marks for aperture selection. This is currently only accessible for users in aperture photometry in Imviz, but is designed to be extended for aperture photometry and spectral extraction (including cone support) in cubeviz (see below for the hidden implementation of cone preview in cubeviz's spectral extraction).

Screen.Recording.2024-01-18.at.9.07.06.AM.mov

Feature-Preview: Cone Apertures in Cubeviz

To enable the cone preview (this is not available to users since it doesn't actually do anything yet, so is hidden behind a "feature flag"):

cubeviz.plugins['Spectral Extraction']._obj.dev_cone_support = True

Screen.Recording.2024-01-18.at.1.40.05.PM.mov

Note that this also renames spatial_subset within spectral extraction to aperture (with a deprecation warning).

TODO:

support for scaling ellipse/rectangle (should fix failing tests)
follow-up issue for subsets becoming composite after already added/selected in dropdown 🐱
follow-up ticket to consider frequency vs wavelength case when enabling unit conversion
additional test coverage if needed

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

jdaviz/core/template_mixin.py

codecov · 2024-01-18T17:10:26Z

Codecov Report

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

Comparison is base (189276d) 90.81% compared to head (c60d54f) 90.85%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
jdaviz/core/template_mixin.py	97.91%	2 Missing ⚠️
...plugins/spectral_extraction/spectral_extraction.py	97.61%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2664      +/-   ##
==========================================
+ Coverage   90.81%   90.85%   +0.04%     
==========================================
  Files         162      162              
  Lines       20962    21125     +163     
==========================================
+ Hits        19036    19194     +158     
- Misses       1926     1931       +5

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

jdaviz/configs/cubeviz/plugins/spectral_extraction/tests/test_spectral_extraction.py

jdaviz/core/template_mixin.py

pllim · 2024-01-19T15:20:35Z

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

@@ -32,7 +33,8 @@


 @tray_registry('imviz-aper-phot-simple', label="Aperture Photometry")
-class SimpleAperturePhotometry(PluginTemplateMixin, DatasetMultiSelectMixin, TableMixin, PlotMixin):
+class SimpleAperturePhotometry(PluginTemplateMixin, ApertureSubsetSelectMixin,


I guess technically this PR would block #2666 since we're touching the same kind of code. It makes sense to get this in first.

Sure, I don't expect much in terms of conflicts though, do you?

Not necessarily, but it does create review churn, as people test our PRs separately and then have to re-test when one is rebased on top of the other one later.

camipacifici · 2024-01-22T15:38:03Z

Minor comment:
I created a subset, then opened aperture photometry. I see the nice marker for the aperture I select. I keep aperture photometry open and go to subset tools to modify the subset I am using. When I modify it, the marker does not follow (which could be ok), but if I go back to aperture photometry and just re-click "calculate", the tool uses the new subset, but the marker does not move. The marker moves only if I change the aperture to another subset and then go back to the one I want.

kecnry · 2024-01-22T17:26:23Z

@camipacifici - fixed, thanks for noticing that!

kecnry · 2024-01-24T14:25:14Z

Moving to draft now that image rotation is merged. This will now need to handle a change to rotation (although change to link type now requires dropping subsets, so some logic for that can be removed).

pllim

As viewer limits readjust (which seems to be much slower when linked by WCS), the outline can disappear. This might confuse the user in thinking that things are broken. Should this behavior be documented as a known thing somewhere in user doc?

Maybe also document that this outline would only appear for the subset selected as aperture, not as background.

https://github.com/spacetelescope/jdaviz/blob/main/docs/imviz/plugins.rst#aperture-photometry

CHANGES.rst

jdaviz/core/template_mixin.py

pllim · 2024-01-24T19:38:11Z

jdaviz/core/template_mixin.py

+
+    To use in a plugin:
+
+    * add ``ApertureSubsetSelectMixin`` as a mixin to the class BEFORE ``DatasetSelectMixin``


Maybe explain here why BEFORE is emphasize here?

because it is necessary to add it before and not after because of inheritance order

pllim · 2024-01-24T19:50:00Z

When I draw the subsets in default orientation, have 2 viewers, then create a N-up E-left orientation, the outline disappears in the first viewer (default orientation) but shows up in the second viewer (N-up E-left). See screenshot.

When I make second viewer to follow default orientation like the first viewer, the outline shows up on both viewers again.

pllim · 2024-01-24T19:58:02Z

When I remove all the data, the outline persists even though aperture selection is gone from the plugin.

pllim · 2024-01-24T20:08:24Z

Seems to work fine in Cubeviz.

But I do wonder if there is a need to outline the entire image when aperture is "Entire Cube" to be consistent. (Maybe not?)

Co-authored-by: P. L. Lim <[email protected]>

kecnry · 2024-01-24T21:00:57Z

As viewer limits readjust (which seems to be much slower when linked by WCS), the outline can disappear. This might confuse the user in thinking that things are broken. Should this behavior be documented as a known thing somewhere in user doc?

I think this will improve greatly as we improve the zoom performance when linked by WCS. But if it doesn't, we can maybe do some visual tricks to make the transition more smooth.

Maybe also document that this outline would only appear for the subset selected as aperture, not as background.

We can do this temporarily, but I have plans down the road to move away from that as we build out spectral extraction and may want to do the same for aperture photometry 🤷‍♂️

When I draw the subsets in default orientation, have 2 viewers, then create a N-up E-left orientation, the outline disappears in the first viewer (default orientation) but shows up in the second viewer (N-up E-left).

I am running into tracebacks from the image rotation PR when trying to reproduce this so am not sure that this PR is to blame or if its a more general bug we have to fix. I'll make sure that ticket exists 🐱 and also covers testing the aperture workflow.

If there is a way to reproduce without tracebacks from the orientation plugin, then this might be to blame and I'm happy to look into it (please provide a recording or script).

When I remove all the data, the outline persists even though aperture selection is gone from the plugin.

Fixed, thanks! (EDIT: except for the resulting test failure.... working on it)

But I do wonder if there is a need to outline the entire image when aperture is "Entire Cube" to be consistent. (Maybe not?)

Yeah, I wasn't sure... we also will need to decide what to do in the future for subsets that are not apertures since spectral extraction will need to be able to select composite subsets, but won't have sub-pixel or wavelength-dependent support for those (most likely). So is the preview intended to show the selection or just to approximately visualize how sub-pixel and wavelength dependence is being handled 🤷‍♂️

pllim · 2024-01-24T22:17:00Z

If there is a way to reproduce without tracebacks from the orientation plugin

I didn't see the traceback. I did this:

Use ImvizDitheredExample notebook.
Load both images.
Open a second viewer and only load one image there.
Link by WCS.
Click N-up E-left button. At this point, first viewer has default orientation and second viewer is N-up E-left.
Draw ellipse in the second viewer.
Open Aperture Photometry plugin and select the ellipse as aperture.
See the problem reported in Aperture live-preview plugin marks #2664 (comment)

(including hiding when no dataset is selected)

kecnry · 2024-01-25T14:09:55Z

Ok, I can reproduce that. The mark does show, its just in the wrong position 🤔 ... can you see anything wrong in the logic for applying wcs in _get_mark_coords?

pllim · 2024-01-25T15:01:22Z

Ah... So I think the calculation needs to be per viewer. Looks to me like you computed the viewer coords for second viewer and then apply it to first viewer. Does that make sense?

kecnry · 2024-01-25T15:27:08Z

Ah... So I think the calculation needs to be per viewer

It should be, it loops through each viewer and re-computes the coords based on viewer.state.reference_data.coords 🤷‍♂️

pllim · 2024-01-25T15:34:49Z

Maybe have to loop in @bmorris3 here. He wrote the reference handling in #2179 .

kecnry · 2024-01-25T17:08:34Z

@pllim - turned out to be a super-simple/stupid fix. Should be working now 🤞

pllim

LGTM now. Thanks!

@pllim

* basic infrastructure for ApertureSelect component with associated marks * multiple subset support * radius_factor support for upcoming cones * multiviewer support * update mark when changing link type * changelog entry * code cleanup * simplify coords logic thanks to suggestion by @pllim * support rectangles and ellipses * and rename radius_factor > scale_factor * integrate into cubeviz spectral extraction for future cone support * basic test coverage for cone visualization * update docstrings * update marks on change to subset * revert simplification so that marks update correctly on change to ref * Apply suggestions from code review Co-authored-by: P. L. Lim <[email protected]> * update marks when selected dataset is changed (including hiding when no dataset is selected) * fix aperture preview in multiple viewers with different rotations --------- Co-authored-by: P. L. Lim <[email protected]>

kecnry added this to the 3.9 milestone Jan 18, 2024

github-actions bot added imviz plugin Label for plugins common to multiple configurations labels Jan 18, 2024

kecnry force-pushed the aperture-marks branch from 7135855 to 9a5420b Compare January 18, 2024 14:31

pllim reviewed Jan 18, 2024

View reviewed changes

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

kecnry force-pushed the aperture-marks branch from dba5bba to 94ad1f2 Compare January 18, 2024 14:48

pllim reviewed Jan 18, 2024

View reviewed changes

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

kecnry force-pushed the aperture-marks branch from 31e57a5 to b8f06d7 Compare January 18, 2024 16:56

kecnry force-pushed the aperture-marks branch 2 times, most recently from ae29000 to becb20c Compare January 18, 2024 19:01

kecnry added the cubeviz label Jan 18, 2024

kecnry force-pushed the aperture-marks branch 4 times, most recently from ec9746d to 8e442e1 Compare January 18, 2024 20:33

kecnry marked this pull request as ready for review January 18, 2024 21:21

kecnry requested review from rosteen, javerbukh, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners January 18, 2024 21:21

pllim reviewed Jan 19, 2024

View reviewed changes

jdaviz/configs/cubeviz/plugins/spectral_extraction/tests/test_spectral_extraction.py Show resolved Hide resolved

pllim reviewed Jan 19, 2024

View reviewed changes

This was referenced Jan 19, 2024

Use upstream feature-flag proposal for aperture marks kecnry/jdaviz#7

Closed

DEV: feature-flag implementation at app-level #2667

Closed

kecnry force-pushed the aperture-marks branch from a29885b to 6c386f1 Compare January 22, 2024 17:26

kecnry added 2 commits January 24, 2024 09:23

update docstrings

d5ff9ec

update marks on change to subset

0dcfd1d

kecnry marked this pull request as draft January 24, 2024 14:25

kecnry force-pushed the aperture-marks branch 2 times, most recently from 1ef8149 to 3d65c12 Compare January 24, 2024 15:12

revert simplification so that marks update correctly on change to ref

d3edf57

kecnry force-pushed the aperture-marks branch from 3d65c12 to d3edf57 Compare January 24, 2024 17:18

kecnry marked this pull request as ready for review January 24, 2024 17:50

pllim reviewed Jan 24, 2024

View reviewed changes

Apply suggestions from code review

8348b5b

Co-authored-by: P. L. Lim <[email protected]>

update marks when selected dataset is changed

9f42104

(including hiding when no dataset is selected)

kecnry force-pushed the aperture-marks branch from e4eff61 to 9f42104 Compare January 25, 2024 13:52

fix aperture preview in multiple viewers with different rotations

c60d54f

pllim approved these changes Jan 25, 2024

View reviewed changes

pllim merged commit 9414cd8 into spacetelescope:main Jan 25, 2024
15 checks passed

kecnry deleted the aperture-marks branch January 26, 2024 13:00

kecnry mentioned this pull request Jan 26, 2024

Add conical aperture support in the spectral extraction plugin #2679

Merged

13 tasks

bmorris3 mentioned this pull request Feb 19, 2024

BUG: Mask cube is ignored in Cubeviz spectral extraction #2714

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aperture live-preview plugin marks #2664

Aperture live-preview plugin marks #2664

kecnry commented Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

pllim Jan 19, 2024

kecnry Jan 19, 2024

pllim Jan 19, 2024

camipacifici commented Jan 22, 2024

kecnry commented Jan 22, 2024

kecnry commented Jan 24, 2024

pllim left a comment

pllim Jan 24, 2024

kecnry Jan 24, 2024

pllim commented Jan 24, 2024 •

edited

Loading

pllim commented Jan 24, 2024

pllim commented Jan 24, 2024

kecnry commented Jan 24, 2024 •

edited

Loading

pllim commented Jan 24, 2024

kecnry commented Jan 25, 2024 •

edited

Loading

pllim commented Jan 25, 2024

kecnry commented Jan 25, 2024 •

edited

Loading

pllim commented Jan 25, 2024

kecnry commented Jan 25, 2024

pllim left a comment


		To use in a plugin:

		* add ``ApertureSubsetSelectMixin`` as a mixin to the class BEFORE ``DatasetSelectMixin``

Aperture live-preview plugin marks #2664

Aperture live-preview plugin marks #2664

Conversation

kecnry commented Jan 18, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

pllim Jan 19, 2024

Choose a reason for hiding this comment

kecnry Jan 19, 2024

Choose a reason for hiding this comment

pllim Jan 19, 2024

Choose a reason for hiding this comment

camipacifici commented Jan 22, 2024

kecnry commented Jan 22, 2024

kecnry commented Jan 24, 2024

pllim left a comment

Choose a reason for hiding this comment

pllim Jan 24, 2024

Choose a reason for hiding this comment

kecnry Jan 24, 2024

Choose a reason for hiding this comment

pllim commented Jan 24, 2024 • edited Loading

pllim commented Jan 24, 2024

pllim commented Jan 24, 2024

kecnry commented Jan 24, 2024 • edited Loading

pllim commented Jan 24, 2024

kecnry commented Jan 25, 2024 • edited Loading

pllim commented Jan 25, 2024

kecnry commented Jan 25, 2024 • edited Loading

pllim commented Jan 25, 2024

kecnry commented Jan 25, 2024

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

pllim commented Jan 24, 2024 •

edited

Loading

kecnry commented Jan 24, 2024 •

edited

Loading

kecnry commented Jan 25, 2024 •

edited

Loading

kecnry commented Jan 25, 2024 •

edited

Loading