Correct sum units for per steradian #2873
Conversation
javerbukh
requested review from
rosteen,
pllim,
kecnry,
bmorris3,
haticekaratay,
cshanahan1 and
gibsongreen
as code owners
github-actions
bot
added
cubeviz
testing
plugin
| new_unit = collapsed_nddata.unit * astropy.units.sr | ||
| collapsed_nddata.unit = new_unit | ||
| if hasattr(collapsed_nddata.uncertainty, 'unit'): | ||
| new_uncert_unit = collapsed_nddata.uncertainty.unit * astropy.units.sr | ||
| collapsed_nddata.uncertainty.unit = new_uncert_unit |
There was a problem hiding this comment.
This conversion needs a multiplication with an area term that has units [sr/pixel]. The pixels have units like [Jy / sr]. If we want to sum over an area and remove the area unit, we need to multiply (Jy / sr) * (sr / pixel). The second term is stored in the pixar_sr keyword, and you can get it with:
self.spectral_cube.meta.get('PIXAR_SR', 1.0)
There was a problem hiding this comment.
There was a problem hiding this comment.
Does the latest commit look correct?
There was a problem hiding this comment.
you need the area of the aperture itself. You can just refer to self.aperture_area_along_spectral directly to get the area (in pixels) and multiply by the PIXAR_SR to get it in sr.
There was a problem hiding this comment.
self.aperture_area_along_spectral is an array, so is the area one of the elements of the array or the elements multiplied together?
Edit: I hadn't had enough coffee, the test data had a spectral axis with a length of 2 and that tripped me up 🤦♂️ I understand now.
There was a problem hiding this comment.
its an area for each slice (since the aperture could be a cone)
There was a problem hiding this comment.
I see that the wavelength_dependent parameter is not used in _extract_from_aperture, does that mean main no longer has that functionality?
There was a problem hiding this comment.
It can probably be removed from the method signature, yes. Wavelength dependence is now being handled by passing reference_spectral_value to get_mask in the aperture_weight_mask and bg_weight_mask properties, respectively (although please feel free to test to make sure its working as expected).
There was a problem hiding this comment.
Hopefully I'm on the right track now. I added a check of the collapsed flux to the test so you can see if the values look correct. The flux from the original collapsed nddata sum array have been multiplied by the self.aperture_area_along_spectral values (in the test case they are 20).
javerbukh
force-pushed
the
spec-extract-sum-units
branch
from
71b83ef to
8e0d989
Compare
| axis=spatial_axes, **kwargs | ||
| ) # returns an NDDataArray | ||
| # Remove per steradian denominator | ||
| if astropy.units.sr in collapsed_nddata.unit.bases: |
There was a problem hiding this comment.
If you want to check that the unit is per solid angle (rather than just checking if the unit contains steradian), you can use core.validunits.check_if_unit_is_per_solid_angle. that would require some additional logic, which you could add to that function, to return what the solid angle unit is to multiply by.
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Outdated
Show resolved
Hide resolved
| collapsed_nddata.unit *= aperture_area.unit | ||
| collapsed_nddata.data[:] *= aperture_area | ||
| # Update uncertainty units as well | ||
| collapsed_nddata.uncertainty[:] *= aperture_area |
There was a problem hiding this comment.
Let's use NDData.multiply() instead (docs).
|
Test failures are present on
2.Run the following lines of code
|
| if astropy.units.sr in collapsed_nddata.unit.bases: | ||
| aperture_area = (self.aperture_area_along_spectral | ||
| * self.spectral_cube.meta.get('PIXAR_SR', 1.0) * u.sr) | ||
| collapsed_nddata = collapsed_nddata.multiply(aperture_area, |
|
|
||
| def test_spectral_extraction_with_correct_sum_units(cubeviz_helper, | ||
| spectrum1d_cube_fluxunit_jy_per_steradian): | ||
| cubeviz_helper.load_data(spectrum1d_cube_fluxunit_jy_per_steradian) |
There was a problem hiding this comment.
This is not strictly necessary, but good for future quality of life. Could we add an assert after this line to ensure that the loaded cube's flux unit is Jy/sr? That would make it really clear what this test is checking for later on.
| @@ -214,7 +214,7 @@ def _create_spectrum1d_cube_with_fluxunit(fluxunit=u.Jy, shape=(2, 2, 4), with_u | |||
| "TELESCOP": "JWST", "BUNIT": fluxunit.to_string(), "PIXAR_A2": 0.01} | |||
| w = WCS(wcs_dict) | |||
| if with_uncerts: | |||
| uncert = StdDevUncertainty(np.abs(np.random.normal(flux) * u.Jy)) | |||
| uncert = StdDevUncertainty(np.abs(np.random.normal(flux) * fluxunit)) | |||
javerbukh
force-pushed
the
spec-extract-sum-units
branch
from
278c7d8 to
b6eb788
Compare
javerbukh
force-pushed
the
spec-extract-sum-units
branch
from
b6eb788 to
d6e370a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2873 +/- ##
=======================================
Coverage 88.78% 88.79%
=======================================
Files 111 111
Lines 17197 17223 +26
=======================================
+ Hits 15269 15293 +24
- Misses 1928 1930 +2 ☔ View full report in Codecov by Sentry. |
| u.Jy/u.sr: u.Unit('W/(m2*sr)'), | ||
| u.Jy/u.sr: u.Unit('W/(m2)'), | ||
| u.Jy: u.Unit('W/m2'), | ||
| u.Unit('W/m2/m')/u.sr: u.Unit('W/(m2*sr)'), | ||
| u.Unit('W/m2/m')/u.sr: u.Unit('W/(m2)'), | ||
| u.Unit('W/m2/m'): u.Unit('W/m2') |
There was a problem hiding this comment.
Hopefully these changes make sense in the context of the sum spectrum using units without per steradian.
| @@ -283,6 +285,73 @@ def standardize_metadata(metadata): | |||
| return out_meta | |||
|
|
|||
|
|
|||
| def flux_conversion(spec, values, original_units, target_units): | |||
There was a problem hiding this comment.
still don't love this name, but I don't exactly have better suggestions that aren't quite verbose. Since this is internal, I think its fine for now and we can always change it later. Can you maybe add a docstring or longer comment explaining what it does though so future us don't get confused?
There was a problem hiding this comment.
Added, let me know if it looks good. If so, I'll merge once CI passes.
Description
This pull request sets the extracted spectrum to the correct units when using the sum function with flux units in per steradian.
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.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.
triviallabel.