lcviz cube support #2678

kecnry · 2024-01-26T16:58:10Z

Description

This pull request:

adds an lcviz exception to which indices from the cube array to pull mouseover information. This would be nice to generalize in a way that the helper or viewer can store the order, but is difficult to do.. Refactor coords_info (mouseover information) so that lcviz can override the dimensions to grab from the cube in make use of upstream refactor to override indices in lcviz lcviz#83.
generalizes plot options stretch histogram to pull from the layer attribute rather than assuming to plot this histogram of the first component
extends on existing lcviz exceptions in the data menu filtering logic to only show TPF entries in cube viewers (and exclude them from light curve viewers) based on data dimensionality

This is not expected to change any behavior in jdaviz itself.

To test in lcviz, use with spacetelescope/lcviz#81

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)?

* would be nice to generalize to read from the helper the expected order of axes

kecnry · 2024-01-26T17:08:06Z

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

+                    if self.app.config == 'lcviz':
+                        value = arr[viewer.state.slices[0], int(round(y)), int(round(x))]
+                    else:
+                        # cubeviz case:
+                        value = arr[int(round(x)), int(round(y)), viewer.state.slices[-1]]


if we store the indices in the helper, is there any way to generalize this line? 🤔

Helpers could have hidden __getitem__-like methods for indexing the source data, right?

ummm, we could maybe monkeypatch it on to the data-layer, but I'm not sure helper-level makes sense since helpers can have multiple types of data each with their own dimensionality, etc.

codecov · 2024-01-26T17:50:20Z

Codecov Report

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

Comparison is base (e6885df) 90.86% compared to head (c6d0f2d) 90.85%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
...nfigs/default/plugins/plot_options/plot_options.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2678      +/-   ##
==========================================
- Coverage   90.86%   90.85%   -0.01%     
==========================================
  Files         162      162              
  Lines       21127    21157      +30     
==========================================
+ Hits        19196    19222      +26     
- Misses       1931     1935       +4

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

pllim · 2024-01-26T19:51:47Z

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

-            # needed for cubeviz
-            ix_shape = 0
-            iy_shape = 1
+            if self.app.config == 'lcviz':


Is there no other way? I don't know how I feel about having special logic for a config that does not exist in this repo. This might be a slippery slope.

see my comment - if you can think of a way to generalize that line if these indices are stored in the helper, then I am happy to do that. The other option would be to refactor so that this part can be overridden without having to copy and paste the rest of the entire method (perhaps a _get_image_layer_data method).

Even if we manage to generalize this part, what about things like this?

} else if (this.$props.viewer.config === 'lcviz') { if (this.$props.viewer.reference.startsWith('tpf')) { multi_select = false }

we have a ticket to refactor that (the data-menu visibility logic) to be more downstream-friendly. Until we can prioritize that, at least these will live in dedicated if-statements 🤷‍♂️

I think if you do if config == "cubeviz" instead, then we don't have to worry about lcviz logic sneaking in here as lcviz then would go the route of "else".

I'd rather refactor - I don't like the idea of having the fallback behavior in jdaviz be the behavior needed by lcviz. That will make it too hard to remove the logic when we do do a refactor and also risks accidentally breaking downstream.

Really, it is only cubeviz that has the weird transposing due to specutils swapaxes. Everything else (like lcviz) should be in else.

So, if we ever have TRampViz, it would theoretically follow the same indexing as lcviz.

Does 1989b85 work for you? Downstream this would then look something like spacetelescope/lcviz#83

Yes, except the variable assignments (another comment I just made).

bmorris3 · 2024-01-26T20:59:39Z

jdaviz/components/viewer_data_select.vue

@@ -127,6 +127,10 @@ module.exports = {
      if (this.$props.viewer.reference === 'spectrum-2d-viewer') {
        multi_select = false
      }
+    } else if (this.$props.viewer.config === 'lcviz') {
+      if (this.$props.viewer.reference.startsWith('tpf')) {


I was going to leave this comment in a review on the lcviz TPF viewer work, but I'll put it here: are we committed to a viewer name starting with tpf? I think we could do better, since the light curve viewer is also showing TPF data. The distinguishing feature is that this corresponds to an image/slice viewer.

nope, suggestions welcomed!

Some words for thought: in Kepler and TESS we call the usual spatial image data product a "postage stamp," for stars in the KIC/TIC. In TESS you can also use tesscut to cutout an analogous small spatial region for the same purpose. I wouldn't want to call the viewer stamp since not all TPF cubes are postage stamps. I like aperture, but I'd be cautious about that, only because we might make an aperture-related plugin, and we wouldn't want to confuse the viewer and plugin names. image is the least ambiguous – this is one of the very few types of images we expect to see in lcviz.

ok, I'll remove this from this PR at least until a decision is made in lcviz. I think image is probably fine as well and we can always change it to be more specific if needed.

This isn't removed yet but I got a ping to review. What is going on? Hope you can clarify. Thanks.

ah I removed it from the other place (title case), I still need it for the if-statements but will update with the new label suggested by @bmorris3 that has been merged for lcviz. Sorry about that!

I think we still need this here until we prioritize a significant refactor to allow downstream overriding.

bmorris3 · 2024-01-26T21:03:13Z

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

+                    if self.app.config == 'lcviz':
+                        value = arr[viewer.state.slices[0], int(round(y)), int(round(x))]
+                    else:
+                        # cubeviz case:
+                        value = arr[int(round(x)), int(round(y)), viewer.state.slices[-1]]


Helpers could have hidden __getitem__-like methods for indexing the source data, right?

pllim · 2024-01-30T15:25:46Z

jdaviz/configs/default/plugins/plot_options/plot_options.py


        if isinstance(data, GroupedSubset):
            # don't update histogram for subsets:
            return

-        comp = data.get_component(data.main_components[0])
+        comp = data.get_component(layer.state.attribute)


Is this really equivalent to main_component[0]?

No, but I think its what we actually want. Shouldn't this always be the plotted data?

I dunno. Further down, there is a block that calls data.get_component under some condition. Will that still be consistent?

That logic probably also could use rewriting to be more specific as well (currently only applies to 2d spectral viewers, afaik), but should not affect this at all (that is for the x-y limit determination, this is for the actual "color" axis of the image which is what should be plotted on the histogram). Feel free to find a case that breaks if you have any concerns!

I am not expert in Plot Options, so this is fine. I won't do this to, say, aperture photometry though, but that is irrelevant here.

layer.state.attribute is the requested column to be plotted, data.main_components[0] is just the first "main" component (which probably is the default column to be plotted, but is not general enough). For the histogram, we definitely want to plot the same data in the histogram as is shown in the image viewer itself, so I think the previous assumption is not general enough.

pllim · 2024-01-30T15:27:20Z

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

@@ -426,7 +430,11 @@ def _image_viewer_update(self, viewer, x, y):
                arr = image.get_component(attribute).data
                unit = image.get_component(attribute).units
                if image.ndim == 3:
-                    value = arr[int(round(x)), int(round(y)), viewer.state.slices[-1]]
+                    if self.app.config == 'lcviz':


Some comment as https://github.com/spacetelescope/jdaviz/pull/2678/files#r1471440871

* which effectively removes all mentions of lcviz from coords-info in jdaviz * downstream implementation in spacetelescope/lcviz#83

pllim · 2024-01-30T18:09:35Z

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

+    def _image_shape_inds(self, image):
+        if image.ndim == 3:
+            # cubeviz case
+            return (0, 1)


Would prefer you keep the ix_shape and iy_shape assignments here instead of tuples without any explanation.

clarified with inline comments

bmorris3

Thanks, this should work!

pllim

Good enough. Nice compromise. Thanks!

* lcviz-case for cube mouseover * stretch histogram - use layer attribute instead of assuming first * refactor coords info so downstream can override * downstream implementation in spacetelescope/lcviz#83

kecnry added 2 commits January 26, 2024 11:52

lcviz-case for cube mouseover

a1eb402

* would be nice to generalize to read from the helper the expected order of axes

stretch histogram - use layer attribute instead of assuming first

7857649

kecnry added no-changelog-entry-needed changelog bot directive Affects-dev changelog bot directive labels Jan 26, 2024

kecnry added this to the 3.9 milestone Jan 26, 2024

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

kecnry mentioned this pull request Jan 26, 2024

TPF viewer spacetelescope/lcviz#81

Merged

4 tasks

kecnry commented Jan 26, 2024

View reviewed changes

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

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

handle data menu visibility for TPF cubes vs light curves

1aa435b

pllim reviewed Jan 26, 2024

View reviewed changes

bmorris3 reviewed Jan 26, 2024

View reviewed changes

remove tpf until finalized in lcviz

f121f04

kecnry requested review from bmorris3 and pllim January 29, 2024 13:56

rename tpf > image

f1d643a

pllim reviewed Jan 30, 2024

View reviewed changes

kecnry mentioned this pull request Jan 30, 2024

make use of upstream refactor to override indices in lcviz spacetelescope/lcviz#83

Merged

refactor coords info so downstream can override

1989b85

* which effectively removes all mentions of lcviz from coords-info in jdaviz * downstream implementation in spacetelescope/lcviz#83

pllim reviewed Jan 30, 2024

View reviewed changes

clarify ix_shape/iy_shape in comments

c6d0f2d

bmorris3 approved these changes Jan 30, 2024

View reviewed changes

kecnry added the Ready for final review label Jan 30, 2024

pllim approved these changes Jan 30, 2024

View reviewed changes

kecnry merged commit 4b7188a into spacetelescope:main Jan 30, 2024
18 of 19 checks passed

kecnry deleted the lcviz-cube-support branch January 30, 2024 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lcviz cube support #2678

lcviz cube support #2678

kecnry commented Jan 26, 2024 •

edited

Loading

kecnry Jan 26, 2024

bmorris3 Jan 26, 2024

kecnry Jan 26, 2024

codecov bot commented Jan 26, 2024 •

edited

Loading

pllim Jan 26, 2024

kecnry Jan 26, 2024

pllim Jan 26, 2024 •

edited

Loading

kecnry Jan 26, 2024 •

edited

Loading

pllim Jan 30, 2024

kecnry Jan 30, 2024

pllim Jan 30, 2024

pllim Jan 30, 2024

kecnry Jan 30, 2024

pllim Jan 30, 2024

bmorris3 Jan 26, 2024

kecnry Jan 26, 2024

bmorris3 Jan 26, 2024

kecnry Jan 29, 2024

pllim Jan 29, 2024

kecnry Jan 29, 2024

bmorris3 Jan 26, 2024

pllim Jan 30, 2024

kecnry Jan 30, 2024

pllim Jan 30, 2024

kecnry Jan 30, 2024

pllim Jan 30, 2024

kecnry Jan 30, 2024

pllim Jan 30, 2024

pllim Jan 30, 2024

kecnry Jan 30, 2024

bmorris3 left a comment

pllim left a comment

lcviz cube support #2678

lcviz cube support #2678

Conversation

kecnry commented Jan 26, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

kecnry Jan 26, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

bmorris3 left a comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading

pllim Jan 26, 2024 •

edited

Loading

kecnry Jan 26, 2024 •

edited

Loading