Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PoC: Cubeviz parser for Roman L1 (ramp) products #2376

Closed
wants to merge 3 commits into from

Conversation

bmorris3
Copy link
Contributor

@bmorris3 bmorris3 commented Aug 15, 2023

Description

Several folks on the Roman team have requested help visualizing WFI Level 1 (image ramp) products. These data are flux cubes with two spatial dimensions and one "sample"/"group"/"resultant" dimension (these three names sometimes have context-dependent meanings, and sometimes are used interchangeably). Typical ramp cubes for each of the 18 detectors have 4086^2 pixels and ~6 samples per pixel.

This PR implements a proof of concept that allows the user to load the ramps into Cubeviz. A dedicated Roman 3D data parser is introduced for Cubeviz, which updates each Cubeviz viewer name by taking advantage of the viewer reference name update support implemented in #2338.

The repurposed Cubeviz viewers for this use case are:

  • "group-viewer": shows the cumulative flux in each pixel at a specific sample-up-the-ramp or one "group"
  • "group-diff-viewer": when group i is selected, this viewer shows the flux integrated between groups i-1 and i. This viewer shows an all-zero slice when i==0.
  • "integration-viewer": by default, shows the median flux of all pixels as a function of group number

Here's what the helper looks like with an L1 file loaded:

cubeviz-roman-ramps.mov

Interactive subsets are supported, showing the median flux in the subset for each sample in the integration-viewer:

cubeviz-roman-ramps-subsets.mov

The single-pixel subset option allows you to interactively view the ramp for single pixels of interest in the integration viewer. This may be helpful e.g. for finding where/when the core of a PSF saturates, or cosmic ray hits:

cubeviz-roman-ramps-single-pixel.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.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

@bmorris3 bmorris3 force-pushed the ramp-nbs-roman-rebased branch 3 times, most recently from 7352138 to e2fe99a Compare August 22, 2023 18:04
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Attention: Patch coverage is 35.20000% with 81 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (358d656) to head (fd66649).
Report is 217 commits behind head on main.

Files with missing lines Patch % Lines
jdaviz/configs/cubeviz/plugins/slice/slice.py 19.56% 37 Missing ⚠️
jdaviz/configs/cubeviz/plugins/parsers.py 27.02% 27 Missing ⚠️
jdaviz/configs/specviz/plugins/viewers.py 72.22% 5 Missing ⚠️
jdaviz/app.py 33.33% 4 Missing ⚠️
...igs/default/plugins/model_fitting/model_fitting.py 70.00% 3 Missing ⚠️
jdaviz/core/template_mixin.py 25.00% 3 Missing ⚠️
...igs/specviz/plugins/line_analysis/line_analysis.py 66.66% 1 Missing ⚠️
jdaviz/core/user_api.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2376      +/-   ##
==========================================
- Coverage   88.79%   88.40%   -0.39%     
==========================================
  Files         111      111              
  Lines       17223    17337     +114     
==========================================
+ Hits        15293    15327      +34     
- Misses       1930     2010      +80     

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


🚨 Try these New Features:

@bmorris3 bmorris3 added this to the 3.7 milestone Aug 25, 2023
@bmorris3 bmorris3 force-pushed the ramp-nbs-roman-rebased branch from e2fe99a to 85791bb Compare August 25, 2023 13:56
@bmorris3 bmorris3 changed the title Cubeviz parser for Roman L1 (ramp) products PoC: Cubeviz parser for Roman L1 (ramp) products Aug 25, 2023
@eteq
Copy link
Contributor

eteq commented Aug 25, 2023

This is some high-level thoughts motivated by some out-of-band conversation with @bmorris3 about this PR:

I think it would be much better from a tecchnical and user-facing perspective to treat this as a separate and distinct helper/viz-tool from cubeviz. The reason why is: I think the value of the fixed-layout viz tools (as opposed to more general "freeform" modes of Glue) is that they give the user useful mental shortcuts for 1) the kind of data that goes into the tool, and 2) the kinds of workflows they want to do with that data. Another way of thinking about is that the answer to the question "what's that viz tool for?" should be answereable in about one sentence. And "visualizing spectral data cubes" is a one sentence answer, whereas "visualizing spectral data cubes and other things that are cubical like ramps, although they're not exactly spectral" is too long of a sentence to count 😉 . So I think it's better to instead have this functionality be in a "RampViz" or simlar so we can actually tell users that they go to rampviz for viewing ramps.

That said, what we do not want to do is duplicate a lot of code unnecessarily. So I think two approaches are possible there:

  1. have there be a "basecubeviz" layout that "cubeviz" and "rampviz" inherit from somehow in its entirerty, or
  2. Copy the layout but have the helper have a base/subclass relationship like the above

(I tilt towards 2 because I'm not sure we have good/well-documented ways to inherit layouts/configurations/etc, but I don't have a strong opinion either way.)

There's also a technical reason to break it out into a separate viz tool: we may want a different set of plugins. While many of them make sense, some of them don't necessarily make sense when the "spectrum" is a ramp, and I can definitely foresee future uses of rampviz that don't make sense for cubeviz. E.g., a plugin that runs the ramp-fitting tool in the pipeline and highlights where various things like cosmic rays appear.

@bmorris3
Copy link
Contributor Author

Thanks for your input @eteq.

And "visualizing spectral data cubes" is a one sentence answer, whereas "visualizing spectral data cubes and other things that are cubical like ramps, although they're not exactly spectral"

It sounds like Cubeviz should be a new base class and it should have a subclass Specviz3D? 😉

@kecnry
Copy link
Member

kecnry commented Aug 25, 2023

Just for public record, I tend to lean in the exact opposite direction and would like to make the case that we avoid splitting into separate configs whenever possible to (a) minimize maintainability overhead on the code and docs, (b) avoid confusion of "which do I use" when its not necessarily obvious, and (c) allow for more flexible workflows.

Instead of more configs, I'd like to see viewers/layout, tools, and "plugins" be more self-aware of the type of data loaded into the app and adjust accordingly. As an example with the current code, we could imagine specviz2d being absorbed into specviz and just have the 2d viewer and spectral extraction plugin know when they're applicable based on whether there is a 2d spectrum loaded in the app or not. To some extent we already handle a simpler version of self-awareness for the spectral axis in pixels vs wavelength, and yet we don't have separate configs for specviz2dwavelength, specviz2dfrequency, and specviz2dpixels.

To make things a bit more messy, this decision is coupled strongly with the helper-vs-plugin-API discussion, as the vision I described above would almost require minimizing plugin/context-specific API calls within the helper in favor of them living in the plugins so they are only exposed when it makes sense to expose them (think of it essentially as a responsive or dynamic helper - although if this is the deal-breaker, I can imagine ways to still have top-level methods which toggle their visibility similar to #2347).

I see the added benefit of jdaviz (on top of glue) as providing data parsers for common data products with automatic linking, context-aware data analysis tools, and maybe sensible viewer layout defaults, but not so much a fixed layout or name for each individual case. Instead of having a separate freeform mode, why not make jdaviz in general more flexible and dynamic?

This is obviously a more general and philosophical discussion than "just" supporting Roman ramps, and also involves considerations about dependency handling, but I do agree that this is a good scenario we can use to think about these ideas and their consequences.

@camipacifici
Copy link
Contributor

camipacifici commented Aug 31, 2023

Thank you @bmorris3 for the prototype! It looks great. Maybe a little slow, but we can talk about performance later.
I triggered an error by trying to zoom vertically in the ramp profile

ValueError                                Traceback (most recent call last)
File ~/opt/miniconda3/envs/pr2376/lib/python3.11/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 ~/opt/miniconda3/envs/pr2376/lib/python3.11/site-packages/jdaviz/components/toolbar_nested.py:186, in NestedJupyterToolbar.vue_select_primary(self, args)
    184         break
    185 else:
--> 186     raise ValueError("could not find previous selection")
    188 if isinstance(self.tools[tool_id], CheckableTool):
    189     # only switch to primary if its actually checkable, otherwise
    190     # just activate once
    191     self.tools_data = {
    192         **self.tools_data,
    193         prev_id: {
   (...)
    200         }
    201     }

ValueError: could not find previous selection

@camipacifici
Copy link
Contributor

Now, about the controversy above, I am no technical expert here, but as a user, I would choose @kecnry's approach over @eteq's approach. To me (user) cubeviz is a cube viewer, whatever that cube contains. I would prefer to have cubeviz smart enough to recognize the type of data I am trowing at it rather than having to choose myself between speccubeviz, gifviz, rampviz, starformationhistoryviz.

I think we do not have to solve the controversy at the moment. The first thing to do it to talk with the Roman team and see what their specific needs are, especially in terms of plugins.

@bmorris3 bmorris3 force-pushed the ramp-nbs-roman-rebased branch from 85791bb to 9c82b2b Compare September 6, 2023 13:44
@pllim pllim modified the milestones: 3.7, 3.8 Sep 21, 2023
@rosteen rosteen modified the milestones: 3.8, 3.9 Nov 29, 2023
@gibsongreen gibsongreen modified the milestones: 3.9, 3.10 Apr 5, 2024
@bmorris3 bmorris3 modified the milestones: 3.10, 3.11 May 3, 2024
@bmorris3 bmorris3 force-pushed the ramp-nbs-roman-rebased branch from 9c82b2b to f200c8d Compare June 26, 2024 15:37
@bmorris3 bmorris3 force-pushed the ramp-nbs-roman-rebased branch from f200c8d to fd66649 Compare July 1, 2024 17:48
@bmorris3 bmorris3 added the proof-of-concept Proof of concept for future reference label Jul 17, 2024
@bmorris3 bmorris3 removed this from the 3.11 milestone Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cubeviz proof-of-concept Proof of concept for future reference specviz
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants