implement unit conversion in specviz2d #3253

gibsongreen · 2024-10-24T14:26:52Z

Description

This pull request is to address implementing the Unit Conversion plugin in Specviz2d.

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

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.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2024-12-19T04:21:59Z

Codecov Report

Attention: Patch coverage is 87.50000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 87.53%. Comparing base (74e537f) to head (d2411c1).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
jdaviz/configs/mosviz/plugins/tools.py	60.00%	6 Missing ⚠️
...plugins/spectral_extraction/spectral_extraction.py	70.00%	3 Missing ⚠️
jdaviz/core/marks.py	92.30%	1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (87.50%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3253      +/-   ##
==========================================
+ Coverage   87.51%   87.53%   +0.02%     
==========================================
  Files         128      128              
  Lines       19901    19957      +56     
==========================================
+ Hits        17416    17469      +53     
- Misses       2485     2488       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gibsongreen · 2024-12-19T21:44:13Z

jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py

+                                self.has_angle = False
+                                self.has_sb = False


We don't append an angle unit in Specviz right now, see the screenshot when the native unit is Jy. This hides both from displaying in the UI and unexposes them from the API.

jdaviz/conftest.py

CHANGES.rst

jdaviz/configs/cubeviz/plugins/spectral_extraction/tests/test_spectral_extraction.py

jdaviz/configs/default/plugins/markers/markers.py

jdaviz/configs/default/plugins/model_fitting/model_fitting.py

kecnry · 2024-12-20T19:03:31Z

jdaviz/configs/default/plugins/model_fitting/model_fitting.py

+        spec = self.dataset.selected_spectrum
+        # Needs Attention:
+        # should conversion occur before or after call to _apply_subset_masks?
+        if spec.flux.unit.to_string != self.app._get_display_unit('flux'):


Isn't this a method? Is it cheaper to compare units instead of casting to string?

Suggested change

if spec.flux.unit.to_string != self.app._get_display_unit('flux'):

if spec.flux.unit.to_string() != self.app._get_display_unit('flux'):

Good catch! Is it worth converting the display units to astropy.Unit or keep to string comparison?

I'm not sure, probably fine as is for now

jdaviz/configs/default/plugins/model_fitting/model_fitting.py

kecnry · 2024-12-20T19:06:02Z

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

+                    self._dict['spectral_axis'] = wave.value
+                    self._dict['spectral_axis:unit'] = wave.unit.to_string()


where were these handled before (I don't see them in the else 🤔)?

X-axis conversions were not previously handled in Specviz2d, so they were previously always using the native units and value when mousing over

kecnry · 2024-12-20T19:07:40Z

jdaviz/configs/mosviz/tests/test_data_loading.py

+    # Note: spectra2d Wave loaded in meters, but we respect one spectral unit, so the meters in
+    # converted to Angstrom (the spectra1d spectral unit).


Can you explain this a little more? Is it that a previous data entry was in angstroms or just that that is the default spectral display unit?

Suggested change

# Note: spectra2d Wave loaded in meters, but we respect one spectral unit, so the meters in

# converted to Angstrom (the spectra1d spectral unit).

# Note: spectra2d Wave loaded in meters, but we respect one spectral unit, so the meters is

# converted to Angstrom (the spectra1d spectral unit).

So the loaded 1D and 2D spectra have different native spectral units, with one in meters and one in angstroms and app-wide we're respecting one choice for spectral unit.

jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py

cshanahan1

When testing this with the specviz2d example notebook, the option to change flux unit disappears when you load the data. The screen shots below show an empty specviz instance, with the 'Flux' option appearing under the unit conversion plugin, and then after loading data when that dropdown disappears

cshanahan1 · 2024-12-23T19:08:04Z

What is included in the scope here for surface brightness logic, including coercing flux units to surface brightness, handling square pixel, and toggling the spectral axis? (Are images of 2d spectra even typically or commonly in surface brightness units?)

gibsongreen · 2024-12-23T19:26:14Z

What is included in the scope here for surface brightness logic, including coercing flux units to surface brightness, handling square pixel, and toggling the spectral axis? (Are images of 2d spectra even typically or commonly in surface brightness units?)

There will be no coercing for pix^2 in Specviz2d, but I think it will be applicable for the image viewers in Mosviz and Imviz.
We only apply the scale factor in Cubeviz since it is stored in the metadata when we extract the cube using the plugin (so no translations here)
I did some research into 2d spectra before starting and to my knowledge, they should only be in Flux units. Specviz2d will still be able to load surface brightness units in steradian, if applicable.

…ectrum

kecnry · 2025-01-24T18:59:19Z

Probably could be a follow-up - but do we want to invert the x-axis on the 2d spectrum to align with the 1d spectrum when switching between wavelength and frequency units?

kecnry

Huge effort - just a few small comments and then I think this is good to go in and build on and hopefully didn't miss too many bugs that might slip through 🤞

kecnry · 2025-01-24T19:05:52Z

jdaviz/configs/mosviz/plugins/tools.py

@@ -18,12 +19,43 @@ def _is_matched_viewer(self, viewer):
        return isinstance(viewer, (MosvizProfile2DView, MosvizProfileView))

    def _map_limits(self, from_viewer, to_viewer, limits={}):


any (reasonable) way to increase test coverage in this method?

I'm not sure if codecov will be satisfied, but, I should be able to add to an existing test where the data_collection entry doesn't have the cids that were looking for.

A second addition could be to manually recreate the conversion with the pixel_to_world_limits and vis versa in the test itself and assert whether the results are the same (but it will pretty much be identical to the logic in the method).

I added a second test for when the data loaded doesn't have the Wave nor Wavelength component (I tried using data_collection[0].remove_component('Wave') to no avail). I'm open to adding the pixel_to_world_limits test if coverage doesn't increase much from this recent test addition.

jdaviz/configs/mosviz/plugins/tools.py

jdaviz/configs/specviz/plugins/line_analysis/tests/test_line_analysis.py

jdaviz/configs/specviz2d/plugins/spectral_extraction/spectral_extraction.py

cshanahan1

Great work! I tested this out as best as I could, and made follow up tickets for the existing issues on main that prevented me from testing some of the functionality. I think that it is OK to merge this with the expectation that things might come up once some of the blocking issues are fixed.

The spectrum viewer and image viewer both pick up changes in flux units, and the mouseover displays the correct units in both viewers. Repeated translations of the spectral axis are buggy, but this is the case on main and I created a follow up ticket for this.
Model fitting. Previews of fit models don't display in the spectrum viewer, which is a pre-existing issue. It looks like the test you wrote for this verifies the unit conversion functionality in the model fitting plugin, and I tested various cases, so I think that is sufficient and if something comes up with the previews after that's fixed we can follow up.
Spectral extraction seems to work with unit conversions, I didn't test this super robustly but in the few cases I tested it with, it works.
The markers table plugin accurately reflects the selected unit, and works between various conversions that I tried.
Line Lists and Line Analysis would benefit from a follow up investigation to see if any unit conversion support is needed, in general.
There is a blocking issue with creating subsets that prevented me from testing some of the plugin functionality on subsets. I am unable to create spectral subsets. If this is not fixed with your linking PR, i'll make a ticket

Approving, pending 3.13 test failure (is this unrelated?)

kecnry · 2025-01-30T12:33:04Z

3.13 dev tests failure is unrelated and was fixed in the bugfix branch by #3402 and will be fixed on main by #3238 (pending upstream support for messaging priority)

github-actions bot added cubeviz specviz imviz plugin Label for plugins common to multiple configurations specviz2d labels Oct 24, 2024

gibsongreen modified the milestones: 4.0.1, 4.1 Oct 24, 2024

rosteen force-pushed the main branch from 40cb333 to a16a3a0 Compare October 29, 2024 19:12

gibsongreen mentioned this pull request Nov 4, 2024

fix flux unit conversions in plugins #3228

Merged

gibsongreen force-pushed the uc-enable-all branch from 50e8194 to b2d178d Compare November 29, 2024 20:27

gibsongreen force-pushed the uc-enable-all branch from 22ca012 to f2920e2 Compare December 11, 2024 22:42

gibsongreen marked this pull request as ready for review December 19, 2024 15:37

gibsongreen requested review from rosteen, javerbukh, pllim, kecnry, bmorris3, haticekaratay and cshanahan1 as code owners December 19, 2024 15:37

gibsongreen commented Dec 19, 2024

View reviewed changes

jdaviz/conftest.py Outdated Show resolved Hide resolved

kecnry reviewed Dec 20, 2024

View reviewed changes

rosteen modified the milestones: 4.1, 4.2 Dec 23, 2024

cshanahan1 reviewed Dec 23, 2024

View reviewed changes

gibsongreen force-pushed the uc-enable-all branch from 663e158 to 15c3514 Compare December 26, 2024 17:57

gibsongreen added 20 commits January 23, 2025 10:30

add test for solid angle equivalency list

aad7c6f

add MosvizProfileView test

acd3693

add change log

72dbf9d

reconcile test failures

05ca476

add API test

e86342b

remove global display unit change handler, syntax error in helpers

b0ed10b

ensure continuum marks don't double translate, test coverage

fdb6a5d

move line analysis test

b538d1e

remove files that weren't removed during rebase

556b66f

fix styling

0758920

add missing import

bb956cc

add spectral unit support, add eqvs to model fit, coords, marks

e08cf1f

fix syntax error

0a3a171

2dviewer spectral conversion, first iteration of test coverage

c838bd5

resolve test failures

c11a3c3

address spec density edge case, test coverage

62998b5

address Kyle's review comments

f170224

resolve test failures, update handle_display_unit for get_selected_sp…

b481be1

…ectrum

ensure spectral axis only passed in correct cases

320d8c8

add uc support for spec extract, support for matched zoom and uc

9e031c9

gibsongreen force-pushed the uc-enable-all branch from 2745eef to 9e031c9 Compare January 23, 2025 15:30

gibsongreen added 2 commits January 23, 2025 14:38

reconcile test failures

338979e

resolve trace conversion failure

3e7d1ae

kecnry reviewed Jan 24, 2025

View reviewed changes

address review comments

d2411c1

cshanahan1 approved these changes Jan 29, 2025

View reviewed changes

kecnry approved these changes Jan 30, 2025

View reviewed changes

gibsongreen merged commit 4ffc3a0 into spacetelescope:main Jan 30, 2025
19 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement unit conversion in specviz2d #3253

implement unit conversion in specviz2d #3253

gibsongreen commented Oct 24, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

gibsongreen Dec 19, 2024

kecnry Dec 20, 2024

gibsongreen Dec 23, 2024

kecnry Jan 3, 2025

kecnry Dec 20, 2024

gibsongreen Dec 23, 2024

kecnry Dec 20, 2024

gibsongreen Dec 23, 2024

cshanahan1 left a comment •

edited

Loading

cshanahan1 commented Dec 23, 2024

gibsongreen commented Dec 23, 2024 •

edited

Loading

kecnry commented Jan 24, 2025

kecnry left a comment

kecnry Jan 24, 2025

gibsongreen Jan 24, 2025

gibsongreen Jan 24, 2025 •

edited

Loading

cshanahan1 left a comment •

edited

Loading

kecnry commented Jan 30, 2025

	if spec.flux.unit.to_string != self.app._get_display_unit('flux'):
	if spec.flux.unit.to_string() != self.app._get_display_unit('flux'):

		self._dict['spectral_axis'] = wave.value
		self._dict['spectral_axis:unit'] = wave.unit.to_string()

		# Note: spectra2d Wave loaded in meters, but we respect one spectral unit, so the meters in
		# converted to Angstrom (the spectra1d spectral unit).

		@@ -18,12 +19,43 @@ def _is_matched_viewer(self, viewer):
		return isinstance(viewer, (MosvizProfile2DView, MosvizProfileView))

		def _map_limits(self, from_viewer, to_viewer, limits={}):

implement unit conversion in specviz2d #3253

implement unit conversion in specviz2d #3253

Conversation

gibsongreen commented Oct 24, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshanahan1 left a comment • edited Loading

Choose a reason for hiding this comment

cshanahan1 commented Dec 23, 2024

gibsongreen commented Dec 23, 2024 • edited Loading

kecnry commented Jan 24, 2025

kecnry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibsongreen Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

cshanahan1 left a comment • edited Loading

Choose a reason for hiding this comment

kecnry commented Jan 30, 2025

gibsongreen commented Oct 24, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

cshanahan1 left a comment •

edited

Loading

gibsongreen commented Dec 23, 2024 •

edited

Loading

gibsongreen Jan 24, 2025 •

edited

Loading

cshanahan1 left a comment •

edited

Loading