Skip to content
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

Fix aperture weight mask with loaded mask cube #3358

Merged
merged 13 commits into from
Dec 16, 2024
2 changes: 1 addition & 1 deletion CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ Cubeviz

- Fixed initializing a Gaussian1D model component when ``Cube Fit`` is toggled on. [#3295]

- Spectral extraction now correctly respects the loaded mask cube. [#3319]
- Spectral extraction now correctly respects the loaded mask cube. [#3319, #3358]

Imviz
^^^^^
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,14 +386,14 @@ def slice_display_unit(self):
return astropy.units.Unit(self.app._get_display_unit(self.slice_display_unit_name))

@property
def mask_non_science(self):
def inverted_mask_non_science(self):
# Aperture masks begin by removing from consideration any pixel
# set to NaN, which corresponds to a pixel on the "non-science" portions
# of the detector. For JWST spectral cubes, these pixels are also marked in
# the DQ array with flag `513`.
return np.logical_not(
np.isnan(self.dataset.selected_obj.flux.value)
).astype(float)
# the DQ array with flag `513`. Also respect the loaded mask, if it exists
rosteen marked this conversation as resolved.
Show resolved Hide resolved
nan_mask = np.isnan(self.dataset.selected_obj.flux.value)
return np.logical_not(np.logical_or(self.mask_cube.get_component('flux').data,
nan_mask)).astype(float)

@property
def aperture_weight_mask(self):
Expand All @@ -402,10 +402,10 @@ def aperture_weight_mask(self):
# wavelength, on the range [0, 1].
if self.aperture.selected == self.aperture.default_text:
# Entire Cube
return self.mask_non_science
return self.inverted_mask_non_science

return (
self.mask_non_science *
self.inverted_mask_non_science *
self.aperture.get_mask(
self.dataset.selected_obj,
self.aperture_method_selected,
Expand Down
25 changes: 23 additions & 2 deletions jdaviz/configs/cubeviz/plugins/tests/test_parsers.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,29 @@ def test_numpy_cube(cubeviz_helper):
assert flux.units == 'ct / pix2'


def test_loading_with_mask(cubeviz_helper):
# This also tests that spaxel is converted to pix**2
custom_spec = Spectrum1D(flux=[[[20, 1], [9, 1]], [[3, 1], [6, 5]]] * u.Unit("erg / Angstrom s cm**2 spaxel"), # noqa
rosteen marked this conversation as resolved.
Show resolved Hide resolved
spectral_axis=[1, 2]*u.AA,
mask=[[[1, 0], [0, 0]], [[0, 0], [0, 0]]])
with warnings.catch_warnings():
warnings.filterwarnings("ignore")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops... didn't even notice the CI. 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whew, figured that out, I'll push a commit shortly fixing tests and addressing your comments.

cubeviz_helper.load_data(custom_spec)
#cubeviz_helper.load_data("https://stsci.box.com/shared/static/gts87zqt5265msuwi4w5u003b6typ6h0.gz", cache=True) # noqa
rosteen marked this conversation as resolved.
Show resolved Hide resolved

uc = cubeviz_helper.plugins['Unit Conversion']
uc.spectral_y_type = "Surface Brightness"

se = cubeviz_helper.plugins['Spectral Extraction']
se.function = "Mean"
se.extract()
extracted = cubeviz_helper.get_data("Spectrum (mean)")
assert_allclose(extracted.flux.value, [6, 2])
assert extracted.unit == u.Unit("erg / Angstrom s cm**2 pix**2")


@pytest.mark.remote_data
def test_manga_cube(cubeviz_helper):
def test_manga_with_mask(cubeviz_helper):
# Remote data test of loading and extracting an up-to-date (as of 11/19/2024) MaNGA cube
# This also tests that spaxel is converted to pix**2
with warnings.catch_warnings():
Expand All @@ -224,7 +245,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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is quite a jump. Does this mean MANGA result has been wrong all this time? 😱

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert extracted_max.unit == u.Unit("erg / Angstrom s cm**2 pix**2")


Expand Down
Loading