Fix aperture weight mask with loaded mask cube #3358
Conversation
rosteen
added
bug
rosteen
requested review from
javerbukh,
pllim,
kecnry,
bmorris3,
haticekaratay,
cshanahan1 and
gibsongreen
as code owners
|
Confirmed that these two tests fail on both |
Co-authored-by: P. L. Lim <[email protected]>
| spectral_axis=[1, 2]*u.AA, | ||
| mask=[[[1, 0], [0, 0]], [[0, 0], [0, 0]]]) | ||
| with warnings.catch_warnings(): | ||
| warnings.filterwarnings("ignore") |
There was a problem hiding this comment.
Since we completely control the data generation here, is it possible to make it such a way that it won't emit warning on load in the first place?
There was a problem hiding this comment.
Yeah, I don't think it's needed here actually. But I'm currently tracking down the bigger issues of this breaking like half the tests somehow.
There was a problem hiding this comment.
Ooops... didn't even notice the CI. 😅
There was a problem hiding this comment.
Whew, figured that out, I'll push a commit shortly fixing tests and addressing your comments.
| @@ -224,7 +242,7 @@ def test_manga_cube(cubeviz_helper): | |||
| se.function = "Mean" | |||
| se.extract() | |||
| extracted_max = cubeviz_helper.get_data("Spectrum (mean)").max() | |||
| assert_allclose(extracted_max.value, 2.836957E-18) | |||
| assert_allclose(extracted_max.value, 5.566169e-18) | |||
There was a problem hiding this comment.
That is quite a jump. Does this mean MANGA result has been wrong all this time? 😱
There was a problem hiding this comment.
Yes, I think that mean extract has been wrong (well, wrong if you don't want to average the masked values into your spectrum...) at least since the spectral extraction was changed.
| se.function = "Mean" | ||
| se.extract() | ||
| extracted = cubeviz_helper.get_data("Spectrum (mean)") | ||
| assert_allclose(extracted.flux.value, [6, 1]) |
There was a problem hiding this comment.
Note for my future self so I don't have to do math again:
- (9 + 3 + 6) / 3 = 6
- (1 + 1 + 1) / 3 = 1
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3358 +/- ##
==========================================
- Coverage 88.78% 88.76% -0.02%
==========================================
Files 125 125
Lines 19161 19227 +66
==========================================
+ Hits 17012 17067 +55
- Misses 2149 2160 +11 ☔ View full report in Codecov by Sentry. |
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
adding regression tests to manga mask
…traction.py Co-authored-by: Brett M. Morris <[email protected]>
Merged! |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
… mask cube Fix aperture weight mask with loaded mask cube (spacetelescope#3358) * Track loaded mask cube in cubeviz * Don't save DQ array as loaded_mask for JWST * Add extraction test on file with mask * Codestyle * Add consideration of loaded mask to aperture weight mask * Remove duplicate test, codestyle * Changelog * Update jdaviz/configs/cubeviz/plugins/tests/test_parsers.py Co-authored-by: P. L. Lim <[email protected]> * Cast this as bool here * Fix tests, address review comments * adding regression tests to manga mask * Update jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py Co-authored-by: Brett M. Morris <[email protected]> --------- Co-authored-by: P. L. Lim <[email protected]> Co-authored-by: Brett M. Morris <[email protected]> Co-authored-by: Brett M. Morris <[email protected]> (cherry picked from commit 92390e2)
… mask cube Fix aperture weight mask with loaded mask cube (spacetelescope#3358) * Track loaded mask cube in cubeviz * Don't save DQ array as loaded_mask for JWST * Add extraction test on file with mask * Codestyle * Add consideration of loaded mask to aperture weight mask * Remove duplicate test, codestyle * Changelog * Update jdaviz/configs/cubeviz/plugins/tests/test_parsers.py Co-authored-by: P. L. Lim <[email protected]> * Cast this as bool here * Fix tests, address review comments * adding regression tests to manga mask * Update jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py Co-authored-by: Brett M. Morris <[email protected]> --------- Co-authored-by: P. L. Lim <[email protected]> Co-authored-by: Brett M. Morris <[email protected]> Co-authored-by: Brett M. Morris <[email protected]> (cherry picked from commit 92390e2)
… mask cube Fix aperture weight mask with loaded mask cube (spacetelescope#3358) * Track loaded mask cube in cubeviz * Don't save DQ array as loaded_mask for JWST * Add extraction test on file with mask * Codestyle * Add consideration of loaded mask to aperture weight mask * Remove duplicate test, codestyle * Changelog * Update jdaviz/configs/cubeviz/plugins/tests/test_parsers.py Co-authored-by: P. L. Lim <[email protected]> * Cast this as bool here * Fix tests, address review comments * adding regression tests to manga mask * Update jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py Co-authored-by: Brett M. Morris <[email protected]> --------- Co-authored-by: P. L. Lim <[email protected]> Co-authored-by: Brett M. Morris <[email protected]> Co-authored-by: Brett M. Morris <[email protected]> (cherry picked from commit 92390e2)
…ded mask cube) (#3360) * Backport PR #3358: Fix aperture weight mask with loaded mask cube Fix aperture weight mask with loaded mask cube (#3358) * Track loaded mask cube in cubeviz * Don't save DQ array as loaded_mask for JWST * Add extraction test on file with mask * Codestyle * Add consideration of loaded mask to aperture weight mask * Remove duplicate test, codestyle * Changelog * Update jdaviz/configs/cubeviz/plugins/tests/test_parsers.py Co-authored-by: P. L. Lim <[email protected]> * Cast this as bool here * Fix tests, address review comments * adding regression tests to manga mask * Update jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py Co-authored-by: Brett M. Morris <[email protected]> --------- Co-authored-by: P. L. Lim <[email protected]> Co-authored-by: Brett M. Morris <[email protected]> Co-authored-by: Brett M. Morris <[email protected]> (cherry picked from commit 92390e2) * Update jdaviz/configs/cubeviz/plugins/tests/test_parsers.py
Updates the value in the test from #3319 to be, I think, correct this time, and adds a simpler test that exposes the problem that I had with that test. I'm out of time to add a changelog, I'll do it tomorrow.