Closed bmorris3 closed 1 month ago
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 24 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
Agreed @rosteen, I wrote the same thing in my notes above:
Well, that you did! Consider me a yes to that question then 🙂
@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.
I broke Rampviz pretty quickly in testing the most recent commit. In the RampvizExample notebook:
1) Zoomed in and made a spatial subset around a star. 2) Panned over to another star and made a second spatial subset. 3) Opened the ramp extraction plugin, selected Subset 1 as spatial aperture. 4) Switched to Subset 2 as the spatial aperture.
This put me in a state of constantly getting IOPub message rate exceeded
messages from the notebook:
IOPub message rate exceeded.
The Jupyter server will temporarily stop sending output
to the client in order to avoid crashing it.
To change this limit, set the config variable
`--ServerApp.iopub_msg_rate_limit`.
Current values:
ServerApp.iopub_msg_rate_limit=1000.0 (msgs[/sec](http://localhost:8888/sec))
ServerApp.rate_limit_window=3.0 (secs)
After restarting the kernel to try to replicate, I didn't hit the IOPub rate error, but instead got into a looping state around showing/zooming to the extraction previews (I think) and had to kill the kernel again:
https://github.com/user-attachments/assets/ca56ba7b-caa2-4797-950f-262b7c35591e
The generalization of the cube helper seems fine and doesn't negatively affect Cubeviz as far as I've been able to tell, but it seems like Rampviz is riding (and falling over) the performance line on my machine. The extraction previews disabled message did show up, but I'm not sure if it was actually not disabled, or if there was some other cause for the loop around zooming.
@rosteen Is this your exact workflow? Seems to work for me:
https://github.com/user-attachments/assets/554d4816-6c31-483c-92b8-c7e60e4eb745
Based on conversations on Slack with @rosteen, it seems like the issue mentioned in his last comment was caused by selecting very large spatial subsets.
I've added a traitlet that limits the number of subset pixels used in the subset preview, and adds a warning in the ramp extraction plugin when that limit is exceeded in 99206ff. If the subset has many pixels, a limited random selection of those pixels within the subset are shown.
@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!
Thanks @pllim, looks like it's all fixed now. Thanks @kecnry and @rosteen!
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):
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.spectrum-viewer
in Cubeviz is an instance ofCubevizProfileView
, which inherits fromSpecvizProfileView
. I created a newJdavizProfileView
class containing any methods previously inSpecvizProfileView
that are not spectrum-specific. The spectrum-specific methods remain inSpecvizProfileView
. I then created a newRampvizProfileView
that inherits fromJdavizProfileView
.cubeviz/plugins
directory, but we could/should? move it todefault/plugins
.Here's what's totally new:
NDDataArrays
into theintegration-viewer
. Getting the JWST parser up to speed will come later.RampvizProfileView
andRampvizImageView
, which are just like their Cubeviz equivalents sans assumptions about spectral data types._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:
integration-viewer
, analogous to spectral subsets in specviz or cubeviz) 🎫Closes #2376
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts, list the proposed change log here for review and add toCHANGES.rst
before merge. If no, maintainer should add ano-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.
trivial
label.