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 display units of moments in velocity #2665

Merged
merged 6 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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: 2 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ Cubeviz
- Fixes Spectral Extraction's assumptions of one data per viewer, and flux data only in
flux-viewer/uncertainty data only in uncert-viewer. [#2646]

- Fixed incorrect output units when Velocity is selected in Moment Maps plugin. [#2665]
pllim marked this conversation as resolved.
Show resolved Hide resolved

Imviz
^^^^^

Expand Down
28 changes: 16 additions & 12 deletions jdaviz/configs/cubeviz/plugins/moment_maps/moment_maps.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@
from pathlib import Path

from astropy import units as u
from astropy.constants import c
from astropy.nddata import CCDData
import numpy as np

from traitlets import Bool, List, Unicode, observe
from specutils import manipulation, analysis
from specutils import manipulation, analysis, Spectrum1D

from jdaviz.core.custom_traitlets import IntHandleEmpty, FloatHandleEmpty
from jdaviz.core.events import SnackbarMessage
Expand Down Expand Up @@ -243,20 +242,25 @@ def calculate_moment(self, add_data=True):
data_wcs = getattr(cube.wcs, 'celestial', None)
if data_wcs:
data_wcs = data_wcs.swapaxes(0, 1) # We also transpose WCS to match.
self.moment = CCDData(analysis.moment(slab, order=n_moment).T, wcs=data_wcs)
if n_moment > 0 and self.output_unit_selected.lower()[0:8] == "velocity":

# Convert spectral axis to velocity units if desired output is in velocity
if n_moment > 0 and self.output_unit_selected.lower().startswith("velocity"):
# Catch this if called from API
if not self.reference_wavelength > 0.0:
raise ValueError("reference_wavelength must be set for output in velocity units.")
power_unit = f"{self.dataset_spectral_unit}{self.n_moment}"
self.moment = np.power(self.moment.convert_unit_to(power_unit), 1/self.n_moment)
self.moment = self.moment << u.Unit(self.dataset_spectral_unit)

ref_wavelength = self.reference_wavelength * u.Unit(self.dataset_spectral_unit)
relative_wavelength = (self.moment-ref_wavelength)/ref_wavelength
in_velocity = c*relative_wavelength
if self.output_unit_selected.lower() == "velocity^n":
in_velocity = np.power(in_velocity, self.n_moment)
self.moment = CCDData(in_velocity, wcs=data_wcs)
slab_sa = slab.spectral_axis.to("km/s", doppler_convention="relativistic",
doppler_rest=ref_wavelength)
slab = Spectrum1D(slab.flux, slab_sa)

# Finally actually calculate the moment
self.moment = analysis.moment(slab, order=n_moment).T
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like intermediate calculations shouldn't change the actual instance attribute, but it is just a personal preference, so no biggie.

# If n>1 and velocity is desired, need to take nth root of result
if n_moment > 0 and self.output_unit_selected.lower() == "velocity":
self.moment = np.power(self.moment, 1/self.n_moment)
# Reattach the WCS so we can load the result
self.moment = CCDData(self.moment, wcs=data_wcs)

fname_label = self.dataset_selected.replace("[", "_").replace("]", "")
self.filename = f"moment{n_moment}_{fname_label}.fits"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ def test_moment_velocity_calculation(cubeviz_helper, spectrum1d_cube, tmpdir):
label_mouseover = cubeviz_helper.app.session.application._tools['g-coords-info']
label_mouseover._viewer_mouse_event(uncert_viewer, {'event': 'mousemove',
'domain': {'x': 0, 'y': 0}})
assert label_mouseover.as_text() == ("Pixel x=00.0 y=00.0 Value -4.14382e+05 m / s",
assert label_mouseover.as_text() == ("Pixel x=00.0 y=00.0 Value -4.14668e+02 km / s",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did the decimal value also change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that I'm not terribly surprised that there's a small (<0.1%) difference between my manual conversion of the result vs converting the spectral axis of each spaxel beforehand. Likely due to my manual calculation before using the simpler optical equation, vs now using the full relativistic convention in the conversion through specutils.

"World 13h39m59.9731s +27d00m00.3600s (ICRS)",
"204.9998877673 27.0001000000 (deg)")

Expand All @@ -174,10 +174,11 @@ def test_moment_velocity_calculation(cubeviz_helper, spectrum1d_cube, tmpdir):
mm.calculate_moment()

label_mouseover._viewer_mouse_event(uncert_viewer, {'event': 'mousemove',
'domain': {'x': 0, 'y': 0}})
assert label_mouseover.as_text() == ("Pixel x=00.0 y=00.0 Value -2.99792e+08 m / s",
"World 13h39m59.9731s +27d00m00.3600s (ICRS)",
"204.9998877673 27.0001000000 (deg)")
'domain': {'x': 1, 'y': 1}})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change it to x=1 y=1? Hard to compare the diff to see if results are the same or not before/after patching.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new moment 2 values are indeed different, that calculation was incorrect before. And I switched to (1,1) because the new value at 0 is 0, which is boring 🙂 .

Copy link
Contributor

Choose a reason for hiding this comment

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

That different, huh? I wonder how many scientists trusted the wrong values blindly. 😱

What about the moment 1 differences above?


assert label_mouseover.as_text() == ("Pixel x=01.0 y=01.0 Value +2.32415e+01 km / s",
"World 13h39m59.9461s +27d00m00.7200s (ICRS)",
"204.9997755344 27.0001999998 (deg)")


def test_write_momentmap(cubeviz_helper, spectrum1d_cube, tmp_path):
Expand Down
Loading