Add Rampviz helper, generalize cube config helper and profile viewer #3120

bmorris3 · 2024-07-29T18:32:39Z

Demo video

Watch on Box. The demo uses an example notebook that's included in this PR, which downloads a Roman L1 file stored on Box.

Description

This PR introduces Rampviz, which had a proof of concept in PR #2376. Rampviz is a helper for visualizing Level 1 data products (IR ramps) from Roman and JWST. Rampviz and Cubeviz share a lot of common infrastructure, since both use 3D data cubes that are viewed as slices in an image viewer, and "profiles" – spectral profiles in Cubeviz and what I'm calling "ramp profiles" in Rampviz.

Here's what I've reorganized or generalized to keep these tools consistent (where possible):

I've moved some of the Cubeviz helper's methods into a new CubeConfigHelper class, and both Cubeviz and Rampviz inherit from the new class. All spectral cube-specific helper methods are introduced in the Cubeviz helper, and the ramp-specific helper methods are in Rampviz.
On main, the spectrum-viewer in Cubeviz is an instance of CubevizProfileView, which inherits from SpecvizProfileView. I created a new JdavizProfileView class containing any methods previously in SpecvizProfileView that are not spectrum-specific. The spectrum-specific methods remain in SpecvizProfileView. I then created a new RampvizProfileView that inherits from JdavizProfileView.
The Slice plugin is common to both rampviz and cubeviz, and I've added some logic to behave correctly for both helpers. So far, I've left the Slice directory in the cubeviz/plugins directory, but we could/should? move it to default/plugins.

Here's what's totally new:

The Rampviz helper is very similar to the Cubeviz helper. Differences include: the default viewer reference names, a simple helper-level ramp cube cache, and a slice method which refers to the "group" axis rather than wavelength
the ramp parser borrows heavily from the Cubeviz helper. The main supported data format is Roman datamodels for now, and it can also read in NDDataArrays into the integration-viewer. Getting the JWST parser up to speed will come later.
the new viewers for Rampviz are RampvizProfileView and RampvizImageView, which are just like their Cubeviz equivalents sans assumptions about spectral data types.
users can extract ramp profiles using the simple stats (median/mean/min/max/sum) on the full cube or on pixels within a subset using the Ramp Extraction plugin. The structure and API of Ramp Extraction is similar to Spectral Extraction in Cubeviz. The main difference is in the _extract_from_aperture method, which uses numpy functions to collapse the subset of a cached ramp cube to a 1D ramp profile. The demo ramp in Rampviz has ten 4k images in the cube. It turns out that our spectral extraction in Cubeviz always makes copies of the spectral and uncertainty cubes before doing an extraction, which is not performant on these larger ramp cubes. By introducing a cache, we get speedups of a factor of 2-5. Switching from the NDData approach in cubeviz for error+mask propagation to this simpler numpy-based approach may not be necessary, and we can use NDData instead if we want to include uncertainties, but I don't believe that's useful for ramp cube subset statistics, so I've dropped it for this plugin.

Deferred to a later PR:

while spatial subsets in the image viewers should work, I'm still working on "temporal subsets" (a span between group indices in the integration-viewer, analogous to spectral subsets in specviz or cubeviz) 🎫
we'll need a logo from Jenn when she's available. I put in the cubeviz logo as a placeholder in the docs where necessary. 🎫
should we allow users to export an extracted ramp profile? There's no standard format for that, but it's trivial to use the NDData writer to make, e.g., a CSV.
Part of the long-term plan is support loading Level 2 ("rate") images into the same helper. I think we might want to use the data associations infrastructure to do that efficiently.
a (working) JWST ramp parser 🎫, see JWST L1 ramp parser for Rampviz #3148
create a generalized cube extraction base class, which we can be the parent class for Spectral, Ramp, and TPF extraction. (see discussion) 🎫
tracking code coverage with optional roman dependencies 🎫

Closes #2376

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

codecov · 2024-07-29T18:53:42Z

Codecov Report

Attention: Patch coverage is 54.73802% with 406 lines in your changes missing coverage. Please review.

Project coverage is 87.20%. Comparing base (95d6008) to head (f796e77).
Report is 134 commits behind head on main.

Files with missing lines	Patch %	Lines
...rampviz/plugins/ramp_extraction/ramp_extraction.py	37.80%	153 Missing ⚠️
jdaviz/configs/rampviz/plugins/parsers.py	17.07%	102 Missing ⚠️
jdaviz/configs/rampviz/plugins/viewers.py	42.85%	44 Missing ⚠️
jdaviz/configs/default/plugins/viewers.py	80.93%	41 Missing ⚠️
jdaviz/configs/rampviz/helper.py	51.42%	17 Missing ⚠️
jdaviz/configs/rampviz/plugins/tools.py	44.82%	16 Missing ⚠️
jdaviz/configs/cubeviz/plugins/slice/slice.py	62.96%	10 Missing ⚠️
jdaviz/app.py	50.00%	6 Missing ⚠️
jdaviz/core/helpers.py	80.95%	4 Missing ⚠️
jdaviz/utils.py	77.77%	4 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3120      +/-   ##
==========================================
- Coverage   88.82%   87.20%   -1.63%     
==========================================
  Files         112      121       +9     
  Lines       17626    18239     +613     
==========================================
+ Hits        15657    15905     +248     
- Misses       1969     2334     +365

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

bmorris3 · 2024-08-01T17:23:59Z

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

I've updated this file's docstring, which had a bunch of pre-existing issues that are unrelated to this PR.

bmorris3 · 2024-08-01T17:30:20Z

docs/index.rst

@@ -55,6 +55,16 @@ Jdaviz

         Jump to Mosviz

+   .. grid-item-card::
+      :img-top: logos/cube.svg


Of course, we don't have a Rampviz logo yet, and won't until we have Jenn back. I'm leaving the cubeviz logo here in the mean time.

bmorris3 · 2024-08-01T17:56:49Z

docs/rampviz/plugins.rst

+Ramp Extraction
+===============
+
+
+


To be added once we converge on the way it works...

bmorris3 · 2024-08-01T17:57:06Z

docs/reference/api_plugins.rst

+.. automodapi:: jdaviz.configs.cubeviz.plugins.spectral_extraction.spectral_extraction
+   :no-inheritance-diagram:
+


Can you believe this was missing!?

bmorris3 · 2024-08-01T17:58:13Z

jdaviz/app.py

+            elif linked_data.ndim == 2:
+                # load a several 1D ramp profiles from a 2D NDDataArray:
+                ref_group_component = dc[0].components[-2]
+                ref_flux_component = dc[0].components[-1]
+                linked_group_component = dc[-1].components[1]
+                linked_flux_component = dc[-1].components[2]
+
+                links = [
+                    LinkSame(ref_group_component, linked_group_component),
+                    LinkSame(ref_flux_component, linked_flux_component)
+                ]
+
+                dc.add_link(links)
+                return


This case may or may not actually be useful if we want to enable loading more than one 1D ramp profile in the integration viewer from a 2D NDDataArray. I'm asking Tom+Derek for advice on whether or not that's plausible to implement.

kecnry

just a few thoughts from a first skim. It would be helpful to have a bullet list of high-level changes, and then how those are planned to be changed in the future (i.e., what will inherit from what, where will there be shared logic, how do we move from here to deconfigging?).

kecnry · 2024-08-02T19:05:58Z

jdaviz/configs/cubeviz/plugins/mixins.py

@@ -14,7 +15,7 @@ def slice_component_label(self):

    @property
    def slice_display_unit_name(self):
-        return 'spectral'
+        return 'spectral' if self.default_class != NDDataArray else 'temporal'


this logic feels a little fragile, would it be better to override in a subclass (both here and in the slice plugin)?

I agree that it's fragile. I hesitate to make a subclass to override only this.

Maybe we can check against the config instead of the default_class?

Sounds good!

kecnry · 2024-08-02T19:07:02Z

jdaviz/configs/cubeviz/plugins/slice/slice.py

-    _cube_viewer_cls = CubevizImageView
-    _cube_viewer_default_label = 'flux-viewer'
+    _cube_viewer_cls = (CubevizImageView, RampvizImageView)
+    _cube_viewer_default_label = None  # must be filled in by helper on initialization


I really don't like the helper being responsible for setting something in the plugin, can this be retrieved from the helper during init instead? Or would a subclass here as well help?

Agreed, this is a good idea. I've reworked this as suggested:

can this be retrieved from the helper during init instead?

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

kecnry · 2024-08-02T19:11:56Z

jdaviz/configs/rampviz/plugins/ramp_extraction/ramp_extraction.py

+class RampExtraction(PluginTemplateMixin, ApertureSubsetSelectMixin,
+                     DatasetSelectMixin, AddResultsMixin):


can this not inherit from spectral extraction? Is there any shared logic that can be moved up?

I'd say it shouldn't inherit from anything spectral, but we could make a CubeExtraction parent to both.

lcviz does currently inherit and tried to make the spectral extraction as generic as possible, but I'd definitely be in favor of a completely generic base class that spectral, ramp, and photometric can all inherit from

I am too, let's do it. Do you want to see that in this ticket/PR, or do you mind if I put that together with this one?

separate is probably fine to keep the diff somewhat manageable

rosteen

My eyes started glazing over 2/3 of the way through, I'll come back later to finish looking through the code - so far I didn't see anything that I object to.

I appreciate you moving ProfileFromCube into the default config - we keep generalizing things (e.g coords_info.py) and leaving them in the original config directories, and I've been wanting to go through and move some things for a while. In that vein, would it make sense to move the slice plugin to the default config at this point now that it supports Cubeviz and Rampviz?

bmorris3 · 2024-08-08T19:04:58Z

Agreed @rosteen, I wrote the same thing in my notes above:

The Slice plugin is common to both rampviz and cubeviz, and I've added some logic to behave correctly for both helpers. So far, I've left the Slice directory in the cubeviz/plugins directory, but we could/should? move it to default/plugins.

rosteen · 2024-08-08T19:25:06Z

Agreed @rosteen, I wrote the same thing in my notes above:

Well, that you did! Consider me a yes to that question then 🙂

rosteen

I mostly made sure this didn't break anything in Cubeviz and read through the code, made a couple minor comments but overall it looks pretty good to me.

FYI I did try out the RampvizExample notebook and it was definitely slow on my machine with one or two things possibly broken - the main one being that deleting a spatial subset doesn't seem to clear the extracted ramps from that subset as I would expect in the profile viewer.

rosteen · 2024-08-09T15:27:35Z

jdaviz/core/marks.py

+        try:
+            self._update_unit(reference_data.get_object(cls=Spectrum1D).spectral_axis.unit)
+        except (TypeError, ValueError):
+            # don't update x units for rampviz, which doesn't convert to Spectrum1D


Would it be possible to have an explicit config check here rather than a try/except that could potentially hide real failures?

I've changed this in the latest commit to check if self.viewer.jdaviz_app.config == 'rampviz', as suggested.

rosteen · 2024-08-09T15:29:08Z

jdaviz/utils.py

+            )
+
+            has_spectral_coords = (
+                any(str(coord).startswith('WAVE') for coord in wcs_coords) or


Maybe also add an or clause for frequency? I'm not sure if the WCS has something like FREQ in that case.

bmorris3 · 2024-08-09T16:34:24Z

@rosteen:

FYI I did try out the RampvizExample notebook and it was definitely slow on my machine with one or two things possibly broken - the main one being that deleting a spatial subset doesn't seem to clear the extracted ramps from that subset as I would expect in the profile viewer.

Good eye, I just caught that in 23d6bd1.

…n hover

kecnry

can't guarantee I've thought about every line, but I think this is good enough to go in and build on top of... we can always iterate on the generalization and rampviz-specifics in subsequent PRs. Thanks!

rosteen

I agree with Kyle, this is looking good enough to get in and iterate on later if needed. Thanks for all the work!

jdaviz/core/helpers.py

pllim · 2024-08-22T01:29:10Z

@bmorris3 please make sure all the other changes from https://github.com/spacetelescope/jdaviz/pull/2094/files are re-applied as well if haven't already. Thanks!

bmorris3 · 2024-08-22T14:49:37Z

Thanks @pllim, looks like it's all fixed now. Thanks @kecnry and @rosteen!

github-actions bot added cubeviz testing plugin Label for plugins common to multiple configurations labels Jul 29, 2024

bmorris3 added the rampviz label Jul 29, 2024

bmorris3 added this to the 4.0 milestone Jul 29, 2024

bmorris3 added the no-changelog-entry-needed changelog bot directive label Jul 29, 2024

bmorris3 force-pushed the cube-base-class branch 2 times, most recently from 7e4f6b6 to 125aa09 Compare July 30, 2024 15:17

bmorris3 added the Upstream fix required label Jul 30, 2024

bmorris3 force-pushed the cube-base-class branch from 125aa09 to 7aaf4d0 Compare August 1, 2024 17:02

bmorris3 commented Aug 1, 2024

View reviewed changes

bmorris3 removed the Upstream fix required label Aug 1, 2024

bmorris3 commented Aug 1, 2024

View reviewed changes

bmorris3 changed the title ~~Add config helper for cubic data (including cubeviz and rampviz)~~ Generalize cube config helper and profile viewer, add Rampviz helper Aug 1, 2024

bmorris3 changed the title ~~Generalize cube config helper and profile viewer, add Rampviz helper~~ Add Rampviz helper, generalize cube config helper and profile viewer Aug 1, 2024

kecnry reviewed Aug 2, 2024

View reviewed changes

bmorris3 force-pushed the cube-base-class branch 2 times, most recently from ccf233a to 282f193 Compare August 7, 2024 20:04

rosteen reviewed Aug 8, 2024

View reviewed changes

rosteen reviewed Aug 9, 2024

View reviewed changes

bmorris3 added the Extra CI Run cron jobs in PR label Aug 9, 2024

bmorris3 marked this pull request as ready for review August 9, 2024 20:30

bmorris3 requested a review from javerbukh as a code owner August 9, 2024 20:30

bmorris3 added 18 commits August 21, 2024 14:06

add config helper for cubic data like cubeviz and rampviz

a8dc574

add base profile viewer class

e1a9fc7

add rampviz to labeler

e0856c5

initial rampviz helper with Roman support + assoc changes

2199c06

adding initial rampviz docs

55a83e5

docstring fixes

f07497a

add rampviz example nb, turn off ramp extract export

fe0f5ad

clean up rampviz hdulist parser

5074beb

Comments from Kyle, plus rebase

c3108c1

adding support for visualizing all pixels in subsets, and per pixel o…

9ad53ed

…n hover

basic mouseover coords info support

a22035e

add ramp subset preview toggle, Roman L1+L2 metadata support

bddc521

docstring fix

e9ccea1

first ramp extraction tests

fb3091c

adding tests for rampviz

e009c1f

home button override, test improvements

614dc8d

working on rampviz test coverage

33f5f49

adding limit to pixels shown in subset preview

b7ad85b

bmorris3 force-pushed the cube-base-class branch from 99206ff to 9d857db Compare August 21, 2024 18:07

kecnry approved these changes Aug 21, 2024

View reviewed changes

Review comments from Kyle

18f8263

bmorris3 force-pushed the cube-base-class branch from 9d857db to 18f8263 Compare August 21, 2024 19:16

rosteen added the Ready for final review label Aug 21, 2024

Review comment from Ricky

662fd86

rosteen approved these changes Aug 21, 2024

View reviewed changes

pllim reviewed Aug 21, 2024

View reviewed changes

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

rebase repair

f796e77

bmorris3 merged commit 0c63463 into spacetelescope:main Aug 22, 2024
17 of 19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Rampviz helper, generalize cube config helper and profile viewer #3120

Add Rampviz helper, generalize cube config helper and profile viewer #3120

bmorris3 commented Jul 29, 2024 •

edited

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading

bmorris3 Aug 1, 2024

bmorris3 Aug 1, 2024

bmorris3 Aug 1, 2024

bmorris3 Aug 1, 2024

bmorris3 Aug 1, 2024

kecnry left a comment

kecnry Aug 2, 2024

bmorris3 Aug 5, 2024

kecnry Aug 19, 2024

bmorris3 Aug 21, 2024

kecnry Aug 2, 2024

bmorris3 Aug 5, 2024

kecnry Aug 2, 2024

bmorris3 Aug 5, 2024

kecnry Aug 5, 2024

bmorris3 Aug 5, 2024

kecnry Aug 8, 2024

rosteen left a comment

bmorris3 commented Aug 8, 2024

rosteen commented Aug 8, 2024

rosteen left a comment •

edited

Loading

rosteen Aug 9, 2024

bmorris3 Aug 21, 2024

rosteen Aug 9, 2024

bmorris3 commented Aug 9, 2024

kecnry left a comment

rosteen left a comment

pllim commented Aug 22, 2024

bmorris3 commented Aug 22, 2024

		.. automodapi:: jdaviz.configs.cubeviz.plugins.spectral_extraction.spectral_extraction
		:no-inheritance-diagram:

		class RampExtraction(PluginTemplateMixin, ApertureSubsetSelectMixin,
		DatasetSelectMixin, AddResultsMixin):

		Ramp Extraction
		===============

Add Rampviz helper, generalize cube config helper and profile viewer #3120

Add Rampviz helper, generalize cube config helper and profile viewer #3120

Conversation

bmorris3 commented Jul 29, 2024 • edited Loading

Demo video

Description

Deferred to a later PR:

Change log entry

Checklist for package maintainer(s)

codecov bot commented Jul 29, 2024 • 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

kecnry left a 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

rosteen left a comment

Choose a reason for hiding this comment

bmorris3 commented Aug 8, 2024

rosteen commented Aug 8, 2024

rosteen left a comment • 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

bmorris3 commented Aug 9, 2024

kecnry left a comment

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

pllim commented Aug 22, 2024

bmorris3 commented Aug 22, 2024

bmorris3 commented Jul 29, 2024 •

edited

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading

rosteen left a comment •

edited

Loading