-
Notifications
You must be signed in to change notification settings - Fork 72
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
Level 1 and Level 2 data product support in Rampviz #3194
base: main
Are you sure you want to change the base?
Conversation
8e01313
to
af4c1ec
Compare
af4c1ec
to
225e901
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3194 +/- ##
==========================================
- Coverage 88.45% 88.39% -0.07%
==========================================
Files 124 124
Lines 18389 18404 +15
==========================================
+ Hits 16266 16268 +2
- Misses 2123 2136 +13 ☔ View full report in Codecov by Sentry. |
This looks good already as is! My preferences are those of a single user. Please gather more preferences especially from the Roman folks.
|
I noticed in the screen recording that the subset layers (for a subset drawn on level 2) are not showing on the group or diff viewer - is this by design? The previews do appear when selecting them as apertures, so it seems they should be within view. |
You can see this in your video as well @bmorris3 but it's worse/more obvious on my machine - after doing a linked zoom (or actually, after doing anything that needs the viewers to refresh, like resizing the windows) the two default image viewers "bounce" 5-10 times. I don't see this behavior in the RampvizExample notebook on main, so I'm not sure if it's an interaction with the new viewer here, or something else. Screen.Recording.2024-09-18.at.1.31.12.PM.mov |
@@ -21,7 +21,7 @@ New Features | |||
|
|||
- The standalone version of jdaviz now uses solara instead of voila, resulting in faster load times. [#2909] | |||
|
|||
- New configuration for ramp/Level 1 data products from Roman WFI and JWST [#3120, #3148, #3167, #3171] | |||
- New configuration for ramp/Level 1 data products from Roman WFI and JWST [#3120, #3148, #3167, #3171, #3194] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also say Level 2 now?
raise ValueError(f"Expected a ramp with NAXIS=4 (with axes:" | ||
|
||
if hdu.header['NAXIS'] == 2: | ||
# this may be a calibrated Level 2 image: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a "may", do we need some sort of if
statement here (rather than waiting until later to check for SCI
ext)?
This PR introduces adaptations to allow Level 2 data products to be loaded into Rampviz, at the same time as Level 1 products.
The demo video below opens L1 and L2 products for the Carina nebula. The L2 product is opened in an image viewer to the right of the usual Rampviz viewers. I've loaded the L2 product with its DQ extension, so you can see where pixels are flagged. I zoom in on a region where pixels are flagged by jump detection (pink DQ overlay), create a subset, and show that the pixels in the subset indeed have a jump.
rampviz-l2-dq-jump.mov
To reproduce locally, run:
Follow-up/to-dos
level-2
viewer. Should we?Change log entry
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, maintainershould 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.
trivial
label.