Data Dropdown Component #1313

kecnry · 2022-05-11T19:11:24Z

Description

This pull request improves the functionality of the per-viewer data dropdown menu. It replaces the checkmarks with the eye icon used throughout the rest of the app, adds a delete button where allowed (plugins handle reacting to deleted data entries if they use the DatasetSelect component), hides the menu entirely in some cases (mosviz viewers except for the 1D spectrum-viewer), and filters to only applicable entries inside the menu itself.

New styling and delete button:

Screen.Recording.2022-05-13.at.1.25.02.PM.mov

Filtered entries for cubeviz (only flux cube is available in spectrum viewer, plugin results are filtered based on data shape/type):

Screen.Recording.2022-05-13.at.1.28.03.PM.mov

Replace vs multi-select:

Screen.Recording.2022-05-13.at.2.38.36.PM-1.mov

Mosviz filtered to active row (including plugin results):

Screen.Recording.2022-05-14.at.12.05.21.PM-1.mov

TODO:

filtered entries based on viewer/viz
mosviz row-filtering logic
determine how to handle plugin output in mosviz
test coverage

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 change log needed? If yes, is it added to CHANGES.rst?
Is a milestone set?
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-05-11T19:21:08Z

Codecov Report

Merging #1313 (95259a1) into main (12d97b8) will increase coverage by 0.03%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##             main    #1313      +/-   ##
==========================================
+ Coverage   84.60%   84.64%   +0.03%     
==========================================
  Files          91       91              
  Lines        7836     7878      +42     
==========================================
+ Hits         6630     6668      +38     
- Misses       1206     1210       +4

Impacted Files	Coverage Δ
jdaviz/configs/mosviz/plugins/viewers.py	`81.09% <40.00%> (-1.30%)`	⬇️
jdaviz/core/template_mixin.py	`92.88% <66.66%> (-0.18%)`	⬇️
jdaviz/app.py	`92.22% <94.11%> (+1.00%)`	⬆️
...default/plugins/gaussian_smooth/gaussian_smooth.py	`96.05% <100.00%> (ø)`
...igs/default/plugins/model_fitting/model_fitting.py	`78.79% <100.00%> (ø)`
jdaviz/configs/mosviz/helper.py	`86.77% <100.00%> (+0.03%)`	⬆️
jdaviz/configs/mosviz/plugins/parsers.py	`90.55% <100.00%> (+0.13%)`	⬆️
...configs/default/plugins/data_tools/file_chooser.py	`65.71% <0.00%> (-3.43%)`	⬇️
...z/configs/default/plugins/line_lists/line_lists.py	`69.35% <0.00%> (+0.47%)`	⬆️
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	`92.22% <0.00%> (+0.67%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12d97b8...95259a1. Read the comment docs.

kecnry · 2022-05-12T15:20:22Z

jdaviz/configs/mosviz/helper.py

+        # expose the row to vue for each of the viewers
+        self.app.state.settings = {**self.app.state.settings, 'mosviz_row': msg.selected_index}


would be nice to store this under something other than "settings", but creating a top-level traitlet for this in the app is too config-specific. Open to other suggestions - but it must be a traitlet that we can pass through into container.vue.

I have no better suggestion. Let's do this for now and see.

jdaviz/configs/mosviz/plugins/parsers.py

kecnry · 2022-05-12T15:21:28Z

jdaviz/components/viewer_data_select.vue

+      if (this.$props.viewer.config === 'mosviz' && item.meta.Plugin === undefined) {
+        return false
+      }


should the delete button be disabled in other cases? Perhaps for all non-plugin data entries? Or at least for the flux cube in cubeviz?

Disabling things to prevent users shooting self on foot (within reason) is probably a good thing.

Agreed... I'm just not sure what cases qualify as that vs an expected use. If the user loads multiple data entries from the API, should they be able to delete those (but maybe just not the last entry which would result in no data in the app)?

Maybe we have to ask POs... 😬

This has since been changed to only show the delete button for plugin outputs - for now. If we want to enable deleting of other data entries, we'll need to think about how to handle linking, etc, and how to prevent the app from getting in an unusable state (perhaps by forbidding deleting the reference data, or the last entry, etc).

kecnry · 2022-05-13T18:02:46Z

jdaviz/app.py

+        if 'Identifier' in component_ids:
+            typ = 'table'
+        elif ndims == 1:
+            typ = '1d spectrum'
+        elif ndims == 2:
+            typ = '2d spectrum' if 'Wavelength' in component_ids else 'image'
+        elif ndims == 3:
+            typ = 'cube'
+        else:
+            typ = 'unknown'


reviewers (cc @pllim): please try to break this logic and preferably come up with a more robust solution

If we want to stick with APE 14, Stuart Mumford said our best bet is the world_axis_physical_types attribute values.

For an image data, Data.coords.world_axis_physical_types gave me ['pos.eq.ra', 'pos.eq.dec'], while the example Mosviz notebook 2d spectrum gave me ['em.wl', None]. Full list of possible values appear to be listed at https://github.com/astropy/astropy/blob/main/astropy/wcs/wcsapi/data/ucds.txt . Do we want to go this route?

I'd be happy with either this or the current WCS-length check. If you'd prefer this, I can switch to it, just let me know!

Let's just stick with what you have unless @astrofrog has a better idea. WCS checks might fail if WCS itself is wonky.

javerbukh · 2022-05-16T18:50:32Z

I hate to be the dark mode police but...the file names in the data drop down are not easily visible.

javerbukh · 2022-05-16T19:08:05Z

I am unable to have a moment map and cube selected in the same image viewer in cubeviz. Is anyone else running into that error or just me?


---------------------------------------------------------------------------
IncompatibleDataException                 Traceback (most recent call last)
File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/ipyvue/VueTemplateWidget.py:60, in Events._handle_event(self, _, content, buffers)
     58     getattr(self, "vue_" + event)(data, buffers)
     59 else:
---> 60     getattr(self, "vue_" + event)(data)

File ~/Documents/jdaviz_dev/jdaviz/jdaviz/app.py:1119, in Application.vue_data_item_selected(self, event)
   1115 else:
   1116     selected_items = list(filter(
   1117         lambda id: id != item_id, viewer_item['selected_data_items']))
-> 1119 self._update_selected_data_items(viewer_id, selected_items)

File ~/Documents/jdaviz_dev/jdaviz/jdaviz/app.py:1164, in Application._update_selected_data_items(self, viewer_id, selected_items)
   1160 active_data_labels.append(label)
   1162 [data] = [x for x in self.data_collection if x.label == label]
-> 1164 viewer.add_data(data)
   1166 add_data_message = AddDataMessage(data, viewer,
   1167                                   viewer_id=viewer_id,
   1168                                   sender=self)
   1169 self.hub.broadcast(add_data_message)

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue_jupyter/view.py:117, in IPyWidgetView.add_data(self, data, color, alpha, **layer_state)
    113 def add_data(self, data, color=None, alpha=None, **layer_state):
    115     data = validate_data_argument(self._data, data)
--> 117     result = super().add_data(data)
    119     if not result:
    120         return

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue/viewers/common/viewer.py:218, in Viewer.add_data(self, data)
    216 with ignore_callback(layer.state, 'zorder'):
    217     self._layer_artist_container.append(layer)
--> 218 layer.update()
    219 self.draw_legend()  # need to be called here because callbacks are ignored in previous step
    221 # Add existing subsets to viewer

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue/utils/matplotlib.py:176, in defer_draw.<locals>.wrapper(*args, **kwargs)
    172 @wraps(func)
    173 def wrapper(*args, **kwargs):
    175     if len(DEFER_DRAW_BACKENDS) == 0:
--> 176         return func(*args, **kwargs)
    178     # Don't recursively defer draws. We just check the first draw_idle
    179     # method since all should be modified in sync.
    180     if isinstance(DEFER_DRAW_BACKENDS[0].draw_idle, DeferredMethod):

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue/viewers/image/layer_artist.py:202, in ImageLayerArtist.update(self, *event)
    200 ARRAY_CACHE.pop(self.state.uuid, None)
    201 PIXEL_CACHE.pop(self.state.uuid, None)
--> 202 self._update_image(force=True)
    203 self.redraw()

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue_jupyter/bqplot/image/layer_artist.py:127, in BqplotImageLayerArtist._update_image(self, force, **kwargs)
    123 changed = self.pop_changed_properties()
    125 if force or any(prop in changed for prop in ('layer', 'attribute',
    126                                              'slices', 'x_att', 'y_att')):
--> 127     self._update_image_data()
    128     force = True  # make sure scaling and visual attributes are updated
    130 if force or any(prop in changed for prop in ('v_min', 'v_max', 'contrast',
    131                                              'bias', 'alpha', 'color_mode',
    132                                              'cmap', 'color', 'zorder',
    133                                              'visible', 'stretch',
    134                                              'bitmap_visible', 'contour_visible')):

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue_jupyter/bqplot/image/layer_artist.py:140, in BqplotImageLayerArtist._update_image_data(self, *args, **kwargs)
    139 def _update_image_data(self, *args, **kwargs):
--> 140     self.composite_image.invalidate_cache()
    141     # if the image data change, the contour lines are invalid
    142     self._contour_line_cache.clear()

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue_jupyter/bqplot/image/frb_mark.py:105, in FRBImage.invalidate_cache(self)
    104 def invalidate_cache(self):
--> 105     self.update()

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue_jupyter/bqplot/image/frb_mark.py:95, in FRBImage.update(self, *args, **kwargs)
     92 bounds = [(ymin, ymax, ny), (xmin, xmax, nx)]
     94 # Get the array and assign it to the artist
---> 95 image = self.array_maker(bounds=bounds)
     96 if image is not None:
     97     with self.hold_sync():

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue/viewers/image/composite_array.py:94, in CompositeArray.__call__(self, bounds)
     91 contrast_bias = ContrastBiasStretch(layer['contrast'], layer['bias'])
     93 if callable(layer['array']):
---> 94     array = layer['array'](bounds=bounds)
     95 else:
     96     array = layer['array']

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue_jupyter/bqplot/image/layer_artist.py:64, in BqplotImageLayerArtist.get_image_data(self, *args, **kwargs)
     62     return None
     63 else:
---> 64     return super().get_image_data(*args, **kwargs)

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue/viewers/image/layer_artist.py:138, in ImageLayerArtist.get_image_data(self, bounds)
    135     return None
    137 try:
--> 138     image = self.state.get_sliced_data(bounds=bounds)
    139 except (IncompatibleAttribute, IndexError):
    140     # The following includes a call to self.clear()
    141     self.disable_invalid_attributes(self.state.attribute)

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue/viewers/image/state.py:452, in BaseImageLayerState.get_sliced_data(self, view, bounds)
    449 # We now get the fixed resolution buffer
    451 if isinstance(self.layer, BaseData):
--> 452     image = self.layer.compute_fixed_resolution_buffer(full_view, target_data=self.viewer_state.reference_data,
    453                                                        target_cid=self.attribute, broadcast=False, cache_id=self.uuid)
    454 else:
    455     image = self.layer.data.compute_fixed_resolution_buffer(full_view, target_data=self.viewer_state.reference_data,
    456                                                             subset_state=self.layer.subset_state, broadcast=False, cache_id=self.uuid)

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue/core/data.py:1985, in Data.compute_fixed_resolution_buffer(self, *args, **kwargs)
   1983 def compute_fixed_resolution_buffer(self, *args, **kwargs):
   1984     from .fixed_resolution_buffer import compute_fixed_resolution_buffer
-> 1985     return compute_fixed_resolution_buffer(self, *args, **kwargs)

File ~/opt/anaconda3/envs/test_prs_2.6/lib/python3.8/site-packages/glue/core/fixed_resolution_buffer.py:235, in compute_fixed_resolution_buffer(data, bounds, target_data, target_cid, subset_state, broadcast, cache_id)
    233     for i in range(target_data.ndim):
    234         if isinstance(bounds[i], tuple) and i not in dimensions_all:
--> 235             raise IncompatibleDataException()
    237 # Ideally, we would use fancy indexing to extract the data, but this can
    238 # be slow on non-SSD drives if using memory-mapped arrays, and also doesn't
    239 # work at all for dask arrays. For now, we therefore extract a sub-region
    240 # from the data before applying fancy indexing if the data is a dask
    241 # array. This won't be very efficient when dealing with 3d fixed
    242 # resolution buffers, but it will at least work as opposed to not.
    244 if target_cid is not None and isinstance(data, Data) and isinstance(data.get_component(target_cid), DaskComponent):
    245 
    246     # Extract sub-region of data first, then fetch exact coordinate values

IncompatibleDataException:

kecnry · 2022-05-16T19:11:38Z

@javerbukh - yes I saw that too, that is the error that I asked about. It doesn't seem to always happen, but does seem to happen more here than in main. I suspect it might have to do with the order messages are processed, but I haven't had the chance to dig too much. If anyone familiar with this part of the code has ideas, please let me know!

pllim · 2022-05-16T19:25:03Z

Is the first selected data automatically become the new reference data? Is there a way to make sure the cube gets selected first before everything else?

If that is not the cause, then we need some debugging to find out what is the pattern that triggers this traceback.

pllim · 2022-05-16T19:27:41Z

Here is a crazy thought: For the more rigid viz configurations like Cubeviz and Mosviz, we can disallow deselection of certain data. For example, the flux cube will always be in the flux viewer, and so on. Then maybe we don't have to worry about order of things if the reference data is always unchanging (if indeed it was the unpredictability of what becomes reference data on select/deselect that causes this problem).

kecnry · 2022-05-16T19:35:42Z

I like that idea, except right now we rely on replacing the default cube in order to show output from the plugins. But if its a matter of what order they get added to the viewer, I can probably add logic here to ensure that IF both a cube and non-cube are checked, that the cube is the first entry (or whatever is needed).

kecnry · 2022-05-16T19:46:27Z

I am now consistently seeing the same behavior on main when plotting a moment map on top of a cube, so will create a separate issue for that as I think this PR just exposed that use-case and I don't think it actually introduced the bug.

EDIT: #1323

jdaviz/app.py

jdaviz/configs/cubeviz/plugins/tests/test_data_selection.py

* plugin output data entries are tied to the row at which the were created * switching rows automatically re-selects _all_ plugin data entries for that given row (and de-selects and filters out all plugin results from the previous row) * model fitting and gaussian smooth now include the data label in the default label (so that switching rows proposes a new default label instead of overwriting)

* menu defaults to only showing those belonging to the current row (including plugin results), but can be expanded to include (and plot) items from other rows * hiding extra rows in the menus automatically "unchecks" them (removes them from the plot)

* see added inline comment

now that spacetelescope#1325 is merged and rebased Co-authored-by: P. L. Lim <[email protected]>

javerbukh

Couldn't break it, great work!

pllim · 2022-05-20T14:05:03Z

@camipacifici claimed she broke it, so let's wait to hear back from her first.

kecnry · 2022-05-20T15:30:26Z

Looks like the NIRISS parser has separate loading logic, so I'll need to parse the row numbers there too. I'll work on that and should get that in before we merge, but everything besides the parser should should remain unaffected.

* and only show "show from other MOS rows" if there are entries... since NIRISS can have only a single image data entry

CHANGES.rst

camipacifici · 2022-05-20T20:51:02Z

Works great now!

pllim

This is a great improvement to Data dropdown menu. Great job!

I don't see "delete" option for spectrum converted to other units. I do not know if this is by design or an oversight.

Some minor comments below.

jdaviz/app.py

pllim · 2022-05-20T20:26:05Z

jdaviz/configs/mosviz/helper.py

+        # expose the row to vue for each of the viewers
+        self.app.state.settings = {**self.app.state.settings, 'mosviz_row': msg.selected_index}


I have no better suggestion. Let's do this for now and see.

jdaviz/configs/mosviz/plugins/viewers.py

pllim

I think I found a bug in Imviz. There is no more Data dropdown in any viewers created after loading the app. As a result, there is no way to interactively display data in the extra viewer(s).

* have to use id instead of reference in the title

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

pllim

Imviz bug is fixed. Thanks!

kecnry added the UI/UX😍 label May 11, 2022

kecnry added this to the 2.6 milestone May 11, 2022

kecnry commented May 12, 2022

View reviewed changes

jdaviz/configs/mosviz/plugins/parsers.py Outdated Show resolved Hide resolved

kecnry commented May 12, 2022

View reviewed changes

kecnry force-pushed the data-dropdown-component branch 2 times, most recently from 7fcf974 to 85cf207 Compare May 12, 2022 15:35

kecnry commented May 13, 2022

View reviewed changes

kecnry force-pushed the data-dropdown-component branch 3 times, most recently from 467335f to 87e8c5d Compare May 16, 2022 17:01

kecnry marked this pull request as ready for review May 16, 2022 17:12

kecnry requested review from duytnguyendtn, rosteen, javerbukh and ojustino as code owners May 16, 2022 17:12

pllim reviewed May 17, 2022

View reviewed changes

jdaviz/app.py Show resolved Hide resolved

pllim reviewed May 17, 2022

View reviewed changes

jdaviz/configs/cubeviz/plugins/tests/test_data_selection.py Outdated Show resolved Hide resolved

kecnry mentioned this pull request May 17, 2022

plotting moment map on top of cube fails #1323

Closed

kecnry force-pushed the data-dropdown-component branch from fc21ddc to d79b7a4 Compare May 17, 2022 15:52

kecnry mentioned this pull request May 18, 2022

ENH: Cubeviz metadata viewer #1325

Merged

11 tasks

javerbukh mentioned this pull request May 19, 2022

Add tool that shows spectrum at spaxel in cubeviz #1317

Merged

15 tasks

kecnry added 11 commits May 19, 2022 13:08

show gaussian smooth entries in both image and spectrum-viewers

77b2db0

data dropdowns: ability to toggle between replace and multi-select

c63baf1

simpler data-type-detection

d2cf752

minor styling updates

16d960f

test coverage

80ceb56

fix dark-mode support

53c04d5

rename test data names to avoid confusion

ec6166f

* see added inline comment

only show delete button for plugin products

3fb74c3

fix test from rebase

f60d51d

kecnry force-pushed the data-dropdown-component branch from 775d6bc to f60d51d Compare May 19, 2022 17:10

simplify WCS access from metadata

44a9ed3

now that spacetelescope#1325 is merged and rebased Co-authored-by: P. L. Lim <[email protected]>

javerbukh approved these changes May 20, 2022

View reviewed changes

fix data-filtering for NIRISS

6eca4b1

* and only show "show from other MOS rows" if there are entries... since NIRISS can have only a single image data entry

kecnry force-pushed the data-dropdown-component branch from 905cd1f to 6eca4b1 Compare May 20, 2022 16:10

pllim reviewed May 20, 2022

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

pllim reviewed May 20, 2022

View reviewed changes

pllim requested changes May 20, 2022

View reviewed changes

kecnry and others added 2 commits May 23, 2022 07:52

fix data menu support for imviz generated viewers

ceacc18

* have to use id instead of reference in the title

Apply suggestions from code review

95259a1

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

kecnry force-pushed the data-dropdown-component branch from a7b14b2 to 95259a1 Compare May 23, 2022 11:55

kecnry requested a review from pllim May 23, 2022 11:56

pllim approved these changes May 23, 2022

View reviewed changes

pllim merged commit af4c974 into spacetelescope:main May 23, 2022

kecnry deleted the data-dropdown-component branch May 23, 2022 13:08

kecnry mentioned this pull request Sep 13, 2022

Imviz: Unable to reload same file multiple times #689

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data Dropdown Component #1313

Data Dropdown Component #1313

kecnry commented May 11, 2022 •

edited by pllim

Loading

codecov bot commented May 11, 2022 •

edited

Loading

kecnry May 12, 2022

pllim May 20, 2022

kecnry May 12, 2022

pllim May 12, 2022

kecnry May 12, 2022

pllim May 13, 2022

kecnry May 17, 2022

kecnry May 13, 2022

pllim May 13, 2022

kecnry May 16, 2022

pllim May 16, 2022

javerbukh commented May 16, 2022

javerbukh commented May 16, 2022

kecnry commented May 16, 2022

pllim commented May 16, 2022

pllim commented May 16, 2022

kecnry commented May 16, 2022

kecnry commented May 16, 2022 •

edited

Loading

javerbukh left a comment

pllim commented May 20, 2022

kecnry commented May 20, 2022

camipacifici commented May 20, 2022

pllim left a comment

pllim May 20, 2022

pllim left a comment

pllim left a comment

		# expose the row to vue for each of the viewers
		self.app.state.settings = {**self.app.state.settings, 'mosviz_row': msg.selected_index}

Data Dropdown Component #1313

Data Dropdown Component #1313

Conversation

kecnry commented May 11, 2022 • edited by pllim Loading

Description

Checklist for package maintainer(s)

codecov bot commented May 11, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javerbukh commented May 16, 2022

javerbukh commented May 16, 2022

kecnry commented May 16, 2022

pllim commented May 16, 2022

pllim commented May 16, 2022

kecnry commented May 16, 2022

kecnry commented May 16, 2022 • edited Loading

javerbukh left a comment

Choose a reason for hiding this comment

pllim commented May 20, 2022

kecnry commented May 20, 2022

camipacifici commented May 20, 2022

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

kecnry commented May 11, 2022 •

edited by pllim

Loading

codecov bot commented May 11, 2022 •

edited

Loading

kecnry commented May 16, 2022 •

edited

Loading