fix SB/flux scale factor #2860

kecnry · 2024-05-08T13:51:02Z

Description

This pull request updates the scale-factor used for converting a spectrum between surface-brightness and flux units to use the wavelength-dependent aperture area (array) instead of the cone's scale factor at the current slice (float).

This should be rebased on top of main after #2859 is merged. See #2859 (comment) for more details.

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)?

gibsongreen · 2024-05-23T20:06:17Z

CHANGES.rst

@@ -49,6 +51,8 @@ Cubeviz
  ``wavelength_unit`` (use ``value_unit``), ``show_wavelength`` (use ``show_value``),
  ``slice`` (use ``value``). [#2878]

+- Spectral Extraction: renamed ``collapse_to_spectrum(...)`` to ``extract(...)``. [#2859]


There was a rebase conflict here, I kept this in (which it should be once background subtraction is merged). If I needs to be taken out, will do

codecov · 2024-05-23T20:58:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.70%. Comparing base (c6e222d) to head (119cfb9).

❗ Current head 119cfb9 differs from pull request most recent head 462df24

Please upload reports for the commit 462df24 to get more accurate results.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2860   +/-   ##
=======================================
  Coverage   88.69%   88.70%           
=======================================
  Files         111      111           
  Lines       17122    17132   +10     
=======================================
+ Hits        15187    15197   +10     
  Misses       1935     1935

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

bmorris3

This looks good. I've made a few suggestions to improve the comments since the unit conversion operations can be tricky even with perfect comments.

bmorris3 · 2024-05-28T15:46:22Z

jdaviz/app.py

+                eqv = []
+                # if spectrum data collection item is in Surface Brightness units
+                if u.sr in spec.unit.bases and len(values) != 2:
+                    # Data item in data collection does not update from conversion/translation.


Suggested change

# Data item in data collection does not update from conversion/translation.

# Data item in data collection will not be updated by conversion/translation.

bmorris3 · 2024-05-28T15:46:49Z

jdaviz/app.py

+                    # App wide orginal data units are used for conversion, orginal_units and
+                    # target_units dicate the conversion to take place.


Suggested change

# App wide orginal data units are used for conversion, orginal_units and

# target_units dicate the conversion to take place.

# App wide original data units are used for conversion, original_units and

# target_units dictate the conversion to take place.

bmorris3 · 2024-05-28T15:47:31Z

jdaviz/app.py

+                    # Data item in data collection does not update from conversion/translation.
+                    # App wide orginal data units are used for conversion, orginal_units and
+                    # target_units dicate the conversion to take place.


Update this comment like the earlier one.

So many typos 😅

bmorris3 · 2024-05-28T15:53:08Z

jdaviz/app.py

+                        pixel_scale_min = (Quantity(spec.meta['_pixel_scale_factor'][0])).value
+                        pixel_scale_max = (Quantity(spec.meta['_pixel_scale_factor'][-1])).value


Does anything guarantee that spec.meta['_pixel_scale_factor'] is sorted? If not, use min and max rather than [0] and [-1].

I was following the convention used for getting the limits of the spectrum axis, and do agree ensuring that these 2 values used are actually the min and max of the scale factor array!

bmorris3 · 2024-05-28T15:53:46Z

Don't forget to add a changelog entry.

…tion

… for y-limits

…ectrum wide conversion condition

cshanahan1

looks good

kecnry added this to the 4.0 milestone May 8, 2024

github-actions bot added cubeviz specviz testing plugin Label for plugins common to multiple configurations labels May 8, 2024

gibsongreen force-pushed the sb_flux_scale_factor branch from 01f0da0 to 1c10f96 Compare May 23, 2024 20:04

gibsongreen reviewed May 23, 2024

View reviewed changes

gibsongreen force-pushed the sb_flux_scale_factor branch from cd44f43 to df4e848 Compare May 24, 2024 14:22

gibsongreen marked this pull request as ready for review May 24, 2024 15:12

gibsongreen requested review from rosteen, javerbukh, pllim, bmorris3, haticekaratay and cshanahan1 as code owners May 24, 2024 15:12

bmorris3 approved these changes May 28, 2024

View reviewed changes

gibsongreen added the bug Something isn't working label May 28, 2024

gibsongreen force-pushed the sb_flux_scale_factor branch from df4e848 to 119cfb9 Compare May 28, 2024 17:17

kecnry added the Ready for final review label May 28, 2024

kecnry and others added 5 commits May 30, 2024 11:02

fix pixel scale factor to use wavelength-dependent aperture areas

b97243c

adjust from incorrect scalar scale factor to correct array implementa…

afcdcb6

…tion

update to_unit test after change from scalar to array scale factor

88dd540

update comments, add changelog, use min/max instead of array indexing…

e1401ba

… for y-limits

update conditional post-rebase to ensure limit updates don't enter sp…

462df24

…ectrum wide conversion condition

gibsongreen force-pushed the sb_flux_scale_factor branch from 119cfb9 to 462df24 Compare May 30, 2024 15:41

cshanahan1 approved these changes May 30, 2024

View reviewed changes

gibsongreen merged commit b3c3ae9 into spacetelescope:main May 31, 2024
15 of 17 checks passed

kecnry deleted the sb_flux_scale_factor branch May 31, 2024 16:40

gibsongreen mentioned this pull request May 31, 2024

Fix unit conversion of limits from Fnu to Flam #2889

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix SB/flux scale factor #2860

fix SB/flux scale factor #2860

kecnry commented May 8, 2024 •

edited by gibsongreen

Loading

gibsongreen May 23, 2024

codecov bot commented May 23, 2024 •

edited

Loading

bmorris3 left a comment

bmorris3 May 28, 2024

bmorris3 May 28, 2024

bmorris3 May 28, 2024

gibsongreen May 28, 2024

bmorris3 May 28, 2024

gibsongreen May 28, 2024

bmorris3 commented May 28, 2024

cshanahan1 left a comment

	# Data item in data collection does not update from conversion/translation.
	# Data item in data collection will not be updated by conversion/translation.

		# App wide orginal data units are used for conversion, orginal_units and
		# target_units dicate the conversion to take place.

		pixel_scale_min = (Quantity(spec.meta['_pixel_scale_factor'][0])).value
		pixel_scale_max = (Quantity(spec.meta['_pixel_scale_factor'][-1])).value

fix SB/flux scale factor #2860

fix SB/flux scale factor #2860

Conversation

kecnry commented May 8, 2024 • edited by gibsongreen Loading

Description

Change log entry

Checklist for package maintainer(s)

gibsongreen May 23, 2024

Choose a reason for hiding this comment

codecov bot commented May 23, 2024 • edited Loading

Codecov Report

bmorris3 left a comment

Choose a reason for hiding this comment

bmorris3 May 28, 2024

Choose a reason for hiding this comment

bmorris3 May 28, 2024

Choose a reason for hiding this comment

bmorris3 May 28, 2024

Choose a reason for hiding this comment

gibsongreen May 28, 2024

Choose a reason for hiding this comment

bmorris3 May 28, 2024

Choose a reason for hiding this comment

gibsongreen May 28, 2024

Choose a reason for hiding this comment

bmorris3 commented May 28, 2024

cshanahan1 left a comment

Choose a reason for hiding this comment

kecnry commented May 8, 2024 •

edited by gibsongreen

Loading

codecov bot commented May 23, 2024 •

edited

Loading