-
Notifications
You must be signed in to change notification settings - Fork 74
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
Generalize cube extract #2939
Generalize cube extract #2939
Conversation
14a099d
to
1d4c6af
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2939 +/- ##
==========================================
+ Coverage 88.91% 88.92% +0.01%
==========================================
Files 111 112 +1
Lines 17365 17396 +31
==========================================
+ Hits 15440 15470 +30
- Misses 1925 1926 +1 ☔ View full report in Codecov by Sentry. |
41df811
to
8a3fc8f
Compare
6fce0b0
to
ebbed5d
Compare
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.
One minor suggestion about some disabled message text, but other than that I think this looks good - less complicated that the diff suggested, mostly just moving things around and renaming.
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
ffa282b
to
b5a2734
Compare
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.
Some superficial comments for readability.
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
e2de7c0
to
c773c2c
Compare
@bmorris3 / @rosteen - added a few more commits since your last reviews if you want to re-review (should be able to filter the diff to just the last ~6 commits) @bmorris3 - addressed your comments. I haven't touched renaming background or wavelength-dependent related methods and attributes that don't need to be used for lcviz ( |
I think it looks ok, my approval can stand. |
* instead of relying on a spectrum viewer existing and being set
* (temporarily to just the first instance, later we'll make this more flexible to allow any number of slice indicator viewers)
to ensure that the preview marks are always visible when first opening
* preview on first plugin open is still not correct, but was not on main either
dadd893
to
620a102
Compare
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.
Thanks @kecnry, looks good 👌🏻
spectral_display_unit, reference_spectral_value=None): | ||
slice_display_unit, spatial_axes=(0, 1), reference_spectral_value=None): | ||
# slice_axis is the remaining axis (of (0, 1, 2)) not included in spatial_axes | ||
slice_axis = 3 - sum(spatial_axes) |
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've tested it, and of course it is less performant – but it's the difference between 40 nanoseconds for the original and 150 ns for my suggestion. I'm not sure anyone will notice. 😉
Description
This pull request generalizes cubeviz's spectral extraction so that it can be re-used by lcviz (photometric extraction of target pixel files) and eventually rampviz. This is milestoned for the 4.0 release, which already contains breaking changes to spectral extraction.
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.