data-menu: add subsets/data #3263

kecnry · 2024-10-31T12:50:07Z

Description

This pull request extends on #3254 by implementing the "+" menu to add data and subsets to the current viewer. This also exposes data_menu.create_subset(subset_type) (to enable the matching subset tool in that viewer's toolbar) and data_menu.add_data(data_label) - currently via data_menu = viz.viewers[viewer_label]._obj.data_menu (although the necessity for the _obj will be removed once the data-menu is ready to be exposed to users.

This PR does include logic so that the listed available data entries are applicable to that specific viewer.

This PR does not include the ability to remove data from the viewer or app in the new data-menu: that will be implemented in #3264. Until then, remove data from the old data-menu or from the API for testing purposes, or create data in a plugin but do not tell it to immediately plot in the viewer.

As in #3254, until finished and officially exposed, the data-menu must be enabled via:

for viewer in viz.viewers.values():
    viewer._obj.data_menu._obj.dev_data_menu = True

Screen.Recording.2024-10-31.at.9.13.47.AM.mov

Screen.Recording.2024-10-31.at.9.14.13.AM.mov

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. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

kecnry · 2024-10-31T18:30:17Z

jdaviz/core/template_mixin.py

+        def is_image_not_spectrum(data):
+            return (is_image(data)
+                    and not getattr(data.coords, 'is_spectral', True))
+
        def is_cube(data):
            return len(data.shape) == 3

+        def is_cube_or_image(data):
+            return len(data.shape) >= 2
+
+        def is_spectrum(data):
+            return (len(data.shape) == 1
+                    and data.coords is not None
+                    and getattr(data.coords, 'is_spectral', True))
+
+        def is_2d_spectrum_or_trace(data):
+            return (data.ndim == 2
+                    and data.coords is not None
+                    and getattr(data.coords, 'is_spectral', True)) or 'Trace' in data.meta


I'm not sure if this is the best way to determine if a data collection is a spectrum or not (as opposed to an image in the 2d case, or a trace or light curve in the 1d case). @pllim suggested checking the presence and dimensions of the flux component, which should work for the trace case, but might not if we ever want to support light curves side-by-side. For now we don't need to worry about light curves though, so if there is a case where the data.coords check would fail (uncalibrated spectra, for example), that might be the better way to go for now?

My other suggestion was to inject such data type into Data metadata on load (in the parser). Then the lookup would always be O(1).

that is probably the cheapest and simplest since we likely have access to the actual specutils object at that piont, the only downside is that we then have to remember to set that metadata for any future tests, etc. I'm honestly not sure which is the best - I'll see if anyone else has thoughts and then we can just pick one and cross our fingers it was the right choice.

Setting the data type in the metadata sounds good to me. It would make things more explicit than checking coords and shape, although maybe we would keep that as a fallback in case the data type is not set.

FWIW in three years when specutils 2 is out we'll have spectral_axis_index in the meta for anything loaded as a Spectrum1D - I'm ok with adding something else to the meta for now if it makes this check easier.

I'm worried that adding something to metadata might blow up a bit in scope - any objections to leaving as is now (unless someone can think of a case that fails this test) and revisit some sort of metadata addition as we generalize parsers, etc, down the road?

That's fine with me, I was just agreeing that could be an ok strategy, not demanding we implement that strategy.

jdaviz/components/data_menu_add_data.vue

* can re-implement during UI tweaks at end of data-menu effort

rosteen · 2024-11-01T14:20:27Z

Starting to test this, it looks like toggling subset visibility is a little wonky:

Screen.Recording.2024-11-01.at.10.18.26.AM.mov

The WCS-only layer showing up when toggling the subset back on also happens on main through the plot options (it goes from "mixed state" to showing everything), we might want to think a little more about how to better handle that layer. But it does seem a little unclear what's happening with the subset when clicking the things in this menu.

kecnry · 2024-11-01T14:27:28Z

it looks like toggling subset visibility is a little wonk

Yes, these are both known bugs filed as a follow-up tickets (delayed state issues and mixed state for image subsets from WCS layer) that likely block enabling the new data-menu for users (and we are hoping is addressed by requests for upstream changes, but if those don't come in time we will need to implement workaround). This PR is specifically for the "+" button in the top right.

javerbukh

LTGM!

rosteen

Works in my testing and I don't see anything in the code that I object to - it's actually a smaller diff than I expected!

kecnry added 6 commits October 31, 2024 08:01

import menu with add data to viewer support

bafeb0a

create subset support

1658f6d

close menu when selecting subset

652cf15

filter available data per-viewer

827763c

basic test coverage

2a3cb49

max-height for add submenu

4398aee

kecnry added the UI/UX😍 label Oct 31, 2024

kecnry added this to the 4.1 milestone Oct 31, 2024

github-actions bot added cubeviz specviz mosviz imviz plugin Label for plugins common to multiple configurations labels Oct 31, 2024

kecnry added 3 commits October 31, 2024 08:51

codestyle

d24ee3d

append to existing changelog entry

6fb6520

fix failing test by generalizing is_not_trace filter

315aa9c

kecnry force-pushed the dm-add-data branch from aca54fa to 2d2b9c1 Compare October 31, 2024 18:26

kecnry commented Oct 31, 2024

View reviewed changes

kecnry added 2 commits October 31, 2024 14:31

data filter for mosviz row

8d31d8f

hide "+" menu if empty

b7bc897

kecnry force-pushed the dm-add-data branch from 2d2b9c1 to b7bc897 Compare October 31, 2024 18:32

kecnry commented Oct 31, 2024

View reviewed changes

jdaviz/components/data_menu_add_data.vue Outdated Show resolved Hide resolved

kecnry marked this pull request as ready for review October 31, 2024 18:42

kecnry requested review from rosteen, javerbukh, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners October 31, 2024 18:42

kecnry mentioned this pull request Oct 31, 2024

data-menu: implement action bar (delete, edit subset, metadata) #3264

Merged

11 tasks

remove strike styling in submenu

69a5eb8

* can re-implement during UI tweaks at end of data-menu effort

improved dark theme support

b2aa734

javerbukh approved these changes Nov 1, 2024

View reviewed changes

kecnry added the Ready for final review label Nov 1, 2024

rosteen approved these changes Nov 1, 2024

View reviewed changes

kecnry merged commit 0094168 into spacetelescope:main Nov 1, 2024
21 of 23 checks passed

kecnry deleted the dm-add-data branch November 1, 2024 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data-menu: add subsets/data #3263

data-menu: add subsets/data #3263

kecnry commented Oct 31, 2024 •

edited

Loading

kecnry Oct 31, 2024

pllim Oct 31, 2024

kecnry Oct 31, 2024

javerbukh Nov 1, 2024

rosteen Nov 1, 2024

kecnry Nov 1, 2024

rosteen Nov 1, 2024

rosteen commented Nov 1, 2024

kecnry commented Nov 1, 2024 •

edited

Loading

javerbukh left a comment

rosteen left a comment

data-menu: add subsets/data #3263

data-menu: add subsets/data #3263

Conversation

kecnry commented Oct 31, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

kecnry Oct 31, 2024

Choose a reason for hiding this comment

pllim Oct 31, 2024

Choose a reason for hiding this comment

kecnry Oct 31, 2024

Choose a reason for hiding this comment

javerbukh Nov 1, 2024

Choose a reason for hiding this comment

rosteen Nov 1, 2024

Choose a reason for hiding this comment

kecnry Nov 1, 2024

Choose a reason for hiding this comment

rosteen Nov 1, 2024

Choose a reason for hiding this comment

rosteen commented Nov 1, 2024

kecnry commented Nov 1, 2024 • edited Loading

javerbukh left a comment

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

kecnry commented Oct 31, 2024 •

edited

Loading

kecnry commented Nov 1, 2024 •

edited

Loading