-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add conical aperture support in the spectral extraction plugin #2679
Conversation
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
d612276
to
3a58f77
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2679 +/- ##
==========================================
- Coverage 90.89% 88.86% -2.04%
==========================================
Files 163 108 -55
Lines 21391 15916 -5475
==========================================
- Hits 19444 14144 -5300
+ Misses 1947 1772 -175 ☔ View full report in Codecov by Sentry. |
ea78c2d
to
a1efbc5
Compare
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
@@ -134,12 +134,15 @@ | |||
&& (bg_selected === 'None' || bg_selected_validity.is_aperture) | |||
&& dev_subpixel_support"> | |||
<v-row> |
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.
<v-row> | |
<v-row v-if="aperture_selected !== 'Entire Cube'"> |
We should also include an in-line warning if !aperture_selected_validity.is_aperture
conflicts with the selection here (for example a composite subset and "subpixel"), disable clicking the extract button, and have checks in the python method that raise an error.
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.
As it currently stands, if you have Subset 1 selected and then you make it a composite subset, the option to make a cone wavelength dependent is removed and you see the following message in the UI: "Subset 1 does not support wavelength dependence (cone support): composite subsets are not supported." Is that sufficient or do you mean to cover another case with this?
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.
oh right - I forgot I did that 😝 That's probably fine for now, and I'll make a note of this in the final UI/UX review (we might instead want to continue showing the dropdown and have an error so that there is a consistent workflow from the API perspective).
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.
I am no good with the vue stuff but now that cylindrical also use the same algorithm as cone, any checks here need updating?
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.
I think this should work with #2684. If anything there needs updating, please open a ticket.
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.vue
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
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.
Questions about the test.
jdaviz/configs/cubeviz/plugins/spectral_extraction/tests/test_spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/tests/test_spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/tests/test_spectral_extraction.py
Outdated
Show resolved
Hide resolved
@rosteen That's strange, I'm not seeing that anymore. Are you sure you have the latest commit? |
@bmorris3 , since you requested changes, can you please re-review? Thanks! |
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.
This looks great, thank you!
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
# But we still need the mean function for everything except flux | ||
collapsed_as_mean = nddata_reshaped.mean(axis=spatial_axes, **kwargs) | ||
|
||
# Then normalize the flux based on the fractional pixel array |
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.
I was hoping all this would happen inside NDData somewhere. Kinda defeats the purpose of using NDData if we still have to do it ourselves here, no, @bmorris3 ?
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
as part of JDAT-4194
|
||
|
||
def _move_spectral_axis(wcs, flux, mask=None, uncertainty=None): | ||
""" | ||
Move spectral axis last to match specutils convention. This | ||
function borrows from: | ||
https://github.com/astropy/specutils/blob/ | ||
6eb7f96498072882c97763d4cd10e07cf81b6d33/specutils/spectra/spectrum1d.py#L185-L225 | ||
""" |
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.
@pllim Did you mean to remove all this?
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.
Yes, it is not used anywhere.
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
and also more clean-ups
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.
I basically co-wrote this PR now, so naturally I am now happy where it stands, so approving. But you should revisit if any vue rules need updating for the GUI checks, see https://github.com/spacetelescope/jdaviz/pull/2679/files#r1506335282
Thanks for your patience, Jesse!
Description
This pull request adds support for fractional pixels using NDData as well as using that feature to create conical apertures in cubeviz using the spectral extraction plugin.
Cone mask at different slices:
Cone mask at slice 100
Cone mask at slice 1000
Cone mask at slice 3000
Not wavelength dependent, wavelength dependent, wavelength dependent at a different slice
cubeviz_cone_aperture_demo_02052024.mp4
TODO:
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.