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

add spectral unit to moment map 0 display unit, simplify logic #3211

Merged
merged 1 commit into from
Oct 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ New Features

- Added flux/surface brightness translation and surface brightness
unit conversion in Cubeviz and Specviz. [#2781, #2940, #3088, #3111, #3113, #3129,
#3139, #3149, #3155, #3178, #3185, #3187, #3190, #3156, #3200, #3192, #3206]
#3139, #3149, #3155, #3178, #3185, #3187, #3190, #3156, #3200, #3192, #3206, #3211]

- Plugin tray is now open by default. [#2892]

Expand Down
45 changes: 22 additions & 23 deletions jdaviz/configs/cubeviz/plugins/moment_maps/moment_maps.py
Copy link
Contributor

@bmorris3 bmorris3 Oct 4, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. fixed, removed Flux from options
  2. i see this correctly removing flux as an option already
  3. I intentionally omitted this because I found it to work without it, and I new @kecnry did some work recently on units with an empty viewer so I figured thats why it was now working

Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
spaxel = u.def_unit('spaxel', 1 * u.Unit(""))
u.add_enabled_units([spaxel])

moment_unit_options = {0: ["Surface Brightness", "Flux"],
moment_unit_options = {0: ["Surface Brightness"],
1: ["Velocity", "Spectral Unit"],
2: ["Velocity", "Velocity^N"]}

Expand Down Expand Up @@ -170,7 +170,7 @@

if isinstance(self.n_moment, str) or self.n_moment < 0:
return
unit_options_index = 2 if self.n_moment > 2 else self.n_moment
unit_options_index = min(self.n_moment, 2)
if self.output_unit_selected not in moment_unit_options[unit_options_index]:
self.output_unit_selected = moment_unit_options[unit_options_index][0]
self.send_state("output_unit_selected")
Expand All @@ -192,7 +192,7 @@
sunit = ""
self.dataset_spectral_unit = sunit
unit_dict["Spectral Unit"] = sunit
unit_dict["Surface Brightness"] = self.app._get_display_unit('sb')
unit_dict["Surface Brightness"] = str(self.moment_zero_unit)

# Update units in selection item dictionary
for item in self.output_unit_items:
Expand Down Expand Up @@ -268,14 +268,12 @@

# slice out desired region
# TODO: should we add a warning for a composite spectral subset?
spec_min, spec_max = self.spectral_subset.selected_min_max(cube)
display_spectral_axis_unit = self.app._get_display_unit(self.slice_display_unit_name)
spec_reg = self.spectral_subset.selected_obj

# Convert units of min and max if necessary
if display_spectral_axis_unit and display_spectral_axis_unit != str(spec_min.unit):
spec_min = spec_min.to(display_spectral_axis_unit, equivalencies=u.spectral())
spec_max = spec_max.to(display_spectral_axis_unit, equivalencies=u.spectral())
slab = manipulation.spectral_slab(cube, spec_min, spec_max)
if spec_reg is None:
slab = cube
else:
slab = manipulation.extract_region(cube, spec_reg)

# Calculate the moment and convert to CCDData to add to the viewers
# Need transpose to align JWST mirror shape: This is because specutils
Expand Down Expand Up @@ -313,19 +311,7 @@
# convert units for moment 0, which is the only currently supported
# moment for using converted units.
if n_moment == 0:
# get display units for moment 0 based on unit conversion plugin selection
# the 0th item of this dictionary is the surface brightness unit, kept
# up to date with choices from the UC plugin
moment_0_display_unit = self.output_unit_items[0]['unit_str']

# convert unit string to Unit so moment map data can be converted
# `display_unit` is the data unit (surface brighntess) translated
# to the choice of selected flux and angle unit from the UC plugin.
display_unit = u.Unit(moment_0_display_unit)

moment_new_unit = display_unit * self.app._get_display_unit('spectral') # noqa: E501

self.moment = self.moment.to(moment_new_unit)
self.moment = self.moment.to(self.moment_zero_unit)

# Reattach the WCS so we can load the result
self.moment = CCDData(self.moment, wcs=data_wcs)
Expand All @@ -344,6 +330,19 @@

return self.moment

@property
def moment_zero_unit(self):
if self.spectrum_viewer.state.x_display_unit is not None:
return (
u.Unit(self.app._get_display_unit('sb')) *
self.spectrum_viewer.state.x_display_unit
)
return ''

@property
def spectral_unit_selected(self):
return self.app._get_display_unit('spectral')

Check warning on line 344 in jdaviz/configs/cubeviz/plugins/moment_maps/moment_maps.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/configs/cubeviz/plugins/moment_maps/moment_maps.py#L344

Added line #L344 was not covered by tests

def vue_calculate_moment(self, *args):
self.calculate_moment(add_data=True)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@
></v-radio>
</v-radio-group>
</v-row>
<v-row v-if="output_unit_selected !== 'Spectral Unit' && output_unit_selected !== 'Flux'">
<v-row v-if="output_unit_selected !== 'Spectral Unit' && output_unit_selected !== 'Surface Brightness'">
<v-text-field
ref="reference_wavelength"
type="number"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -347,9 +347,9 @@ def test_correct_output_spectral_y_units(cubeviz_helper, spectrum1d_cube_custom_
# in this list. also check that the initial unit is correct.
output_unit_moment_0 = mm.output_unit_items[0]
assert output_unit_moment_0['label'] == 'Surface Brightness'
assert output_unit_moment_0['unit_str'] == 'MJy / sr'
assert output_unit_moment_0['unit_str'] == 'MJy m / sr'

# check that calculated moment has the correct units
# check that calculated moment has the correct units of MJy m / sr
mm.calculate_moment()
assert mm.moment.unit == f'M{moment_unit}'

Expand All @@ -360,8 +360,16 @@ def test_correct_output_spectral_y_units(cubeviz_helper, spectrum1d_cube_custom_
# and make sure this change is propogated
output_unit_moment_0 = mm.output_unit_items[0]
assert output_unit_moment_0['label'] == 'Surface Brightness'
assert output_unit_moment_0['unit_str'] == 'Jy / sr'
assert output_unit_moment_0['unit_str'] == 'Jy m / sr'

# and that calculated moment has the correct units
mm.calculate_moment()
assert mm.moment.unit == moment_unit

# now change the spectral unit and make sure that change is
# reflected in MM plugin
uc.spectral_unit = 'um'
assert output_unit_moment_0['unit_str'] == 'Jy um / sr'

mm.calculate_moment()
assert mm.moment.unit == moment_unit.replace('m', 'um')
6 changes: 2 additions & 4 deletions jdaviz/core/template_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -2953,8 +2953,7 @@ def _get_continuum(self, dataset, spectral_subset, update_marks=False, per_pixel
if per_pixel:
if self.app.config != 'cubeviz':
raise ValueError("per-pixel only supported for cubeviz")
full_spectrum = self.app._jdaviz_helper.get_data(self.dataset.selected,
use_display_units=True)
full_spectrum = self.app._jdaviz_helper.get_data(self.dataset.selected)
Copy link
Member

Choose a reason for hiding this comment

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

is this (and the other removal below) because the marks are handling the unit conversion themselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i believe so (but also cc @bmorris3) . I also verified this change doesn't break anything in line analysis where this mixin is also used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also see https://jira.stsci.edu/browse/JDAT-4830, this bug was not introduced with this change

Copy link
Member

Choose a reason for hiding this comment

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

see #3212 for the fix to moment map continuum previews during unit conversion. Whichever PR makes it in second should rebase and ensure that is still working as expected.

else:
full_spectrum = dataset.get_selected_spectrum(use_display_units=True)

Expand All @@ -2978,8 +2977,7 @@ def _get_continuum(self, dataset, spectral_subset, update_marks=False, per_pixel
spectrum = full_spectrum
else:
sr = self.app.get_subsets(spectral_subset.selected,
simplify_spectral=True,
use_display_units=True)
simplify_spectral=True)
spectrum = extract_region(full_spectrum, sr, return_single_spectrum=True)
sr_lower = np.nanmin(spectrum.spectral_axis[spectrum.spectral_axis >= sr.lower]) # noqa
sr_upper = np.nanmax(spectrum.spectral_axis[spectrum.spectral_axis <= sr.upper]) # noqa
Expand Down
Loading