Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add conical aperture support in the spectral extraction plugin #2679
Add conical aperture support in the spectral extraction plugin #2679
Changes from 15 commits
f35f7bc
775a9ca
a1efbc5
2e523b3
1a3800b
6ae4b15
ef5e97a
3b8a3b3
defe3b6
a2b7a35
0a6468f
7eabb16
59a3acc
f3a0118
6ab130c
42df577
1428cb6
24ee0ae
fb54e88
c9c6c47
9f854d3
305890a
20e4bf2
7be10fc
4727f7d
239d7f4
8bf7b55
2621fa6
7cb5328
ba90b05
0bb158a
0c1ed55
c63cd0d
2350c72
76ac070
0c968ae
5b575c5
2fd9e63
2e47c77
beed652
497e5f9
8fc323f
2c1dae4
5670532
1d7944d
3efe0ff
f26ab36
da78654
a3ad411
628b919
40c3fed
97090bb
e43992e
0827ee6
26043c7
7816905
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 373 in jdaviz/configs/cubeviz/plugins/parsers.py
jdaviz/configs/cubeviz/plugins/parsers.py#L373
Check warning on line 376 in jdaviz/configs/cubeviz/plugins/parsers.py
jdaviz/configs/cubeviz/plugins/parsers.py#L375-L376
Check warning on line 429 in jdaviz/configs/cubeviz/plugins/parsers.py
jdaviz/configs/cubeviz/plugins/parsers.py#L429
Check warning on line 431 in jdaviz/configs/cubeviz/plugins/parsers.py
jdaviz/configs/cubeviz/plugins/parsers.py#L431
Check warning on line 480 in jdaviz/configs/cubeviz/plugins/parsers.py
jdaviz/configs/cubeviz/plugins/parsers.py#L479-L480
Check warning on line 230 in jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py#L229-L230
Check warning on line 233 in jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py#L233
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 is wrong unless the uncertainty type is normalized outside of the parser code. Here we allow at least 3 different types of uncertainty (inverse variance, variance, standard deviation):
jdaviz/jdaviz/configs/cubeviz/plugins/parsers.py
Line 20 in 07de883
But looks like we just parse those in blindly as-is. How do we know for sure at this point that the uncertainty is always standard deviation?
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.
To this effect, we also need to double check that mask values are being used as intended. Usually telescope ship the MASK as DQ arrays on bitplane, not pure boolean. For display purpose, does not matter, but now that you are passing it into some sort of extraction, how is this mask actually used? Not all "bad" pixels have the same level of badness. Here are example bitplane values.
https://github.com/spacetelescope/stginga/blob/master/stginga/data/dqflags_jwst.txt
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 opened 2 follow-up issues:
Check warning on line 313 in jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py#L313
Check warning on line 318 in jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py#L317-L318
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 still don't understand why the mask handling here is different from the "not cone" case. Why the inconsistency?
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.
We use a different mask handling because the aperture of the cone is different at each slice, where as when using a spatial subset that mask is the same throughout the cube.
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.
But mask cube was not even consider for the other case.
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.
Because the mask is provided by the spatial subset. The cone aperture just uses the center and radius from the subset and calculates the mask based on the mask cube.
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.
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.
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.
What does ones mean here? Are they masked out or not?
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.
Set flux = 1 MJy/sr everywhere in a fake cube. Extracted flux should equal (lambda/lambda_ref) * pi * r_ref^2
I will try to do this in a notebook...
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.
Not sure what
r_ref
is. A complete notebook would be nice. 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.
Can't we just check against the original input?
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 comment is not resolved.