convert limits on spectrum y-unit change #3335

kecnry · 2024-12-03T21:30:21Z

Description

This pull request implements custom logic to convert (instead of resetting) y-limits on a spectrum when the y display unit is changed. This does so by sending all 4 corners through the flux/SB conversion logic and ensuring that those bounds remain in view. As a consequence, multiple unit conversions can result in "zooming out", but never zooming in (i.e. this is not expected to roundtrip, but is supposed to keep any feature in frame).

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

kecnry · 2024-12-03T21:30:52Z

jdaviz/core/freezable_state.py

+                              flux=y_corners * u.Unit(old_unit))
+            y_corners_new = flux_conversion(y_corners, old_unit, new_unit, spec)
+
+            #with delay_callback(self, 'y_min', 'y_max'):


@astrofrog - any idea why including this delay_callback would result in y_max being immediately reverted to zero?

weird, I don't see this as an issue anymore locally or in tests - I guess some other change may have affected it 🤷‍♂️ If anyone does see this happen, please let me know!

gibsongreen · 2024-12-05T14:01:52Z

jdaviz/core/freezable_state.py

+            spec = Spectrum1D(spectral_axis=x_corners * u.Unit(self.x_display_unit),
+                              flux=y_corners * u.Unit(old_unit))


There is a way where we can avoid creating a Spectrum1D object here and just pass the x_corners as a Quantity object to flux_conversion and have it generate the necessary equivalency to be able to do the conversion:
1b80ede

that feels quite hacky to call that slice, but I guess maybe its ok since its just internal. Definitely like saving the overhead of creating the spectrum1d object just to pass quantity arrays.

slice has been now been updated to spectral_axis and I fixed a bug I introduced in flux_conversion, just passing the quantity array is now working!

gibsongreen · 2024-12-05T14:08:33Z

Maybe in test_unit_conversion.py it would be worth adding test coverage to ensure that after a conversion that the minimum y-value of the spectrum is greater than the viewer's minimum y-limit value and the spectrum maximum y-value is less than the viewer y maximum limit.

kecnry · 2024-12-05T17:54:54Z

jdaviz/utils.py

+        if eqv:
+            image_data = True
+            # Needed to convert Flux to Flux for complex conversion/translation of cube image data
+            eqv += u.spectral_density(spectral_axis)
+        elif len(values) == len(spectral_axis):
+            eqv = u.spectral_density(spectral_axis)


@gibsongreen should there be an else here that raises an exception? Can spectral_axis be a single value?

I recall trying to avoid throwing exceptions here but in this case it probably makes sense to have it in the nested conditions.

Originally slice was used for a single value, since Quantity objects can be both scalar or array-like generalizing this to check for both would be best.

codecov · 2024-12-10T17:39:54Z

Codecov Report

Attention: Patch coverage is 89.18919% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.78%. Comparing base (6c946d3) to head (7e297dd).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
jdaviz/utils.py	78.94%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3335      +/-   ##
==========================================
- Coverage   88.80%   88.78%   -0.03%     
==========================================
  Files         125      125              
  Lines       19137    19187      +50     
==========================================
+ Hits        16995    17035      +40     
- Misses       2142     2152      +10

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

astrofrog

This seems reasonable to me - I don't understand why delay_callback wouldn't work though, I can try and investigate this.

astrofrog · 2024-12-12T23:42:06Z

It might be worth adding a test for this with an equivalency that does affect the final limits?

kecnry · 2024-12-13T00:21:15Z

Yep, tests are coming soon.

cshanahan1 · 2024-12-13T16:49:57Z

jdaviz/core/freezable_state.py

+            else:
+                spectral_axis.info.meta = {}
+
+            y_corners_new = flux_conversion(y_corners, old_unit, new_unit, spectral_axis=spectral_axis)  # noqa


will this handle equivalencies needed for surface brightness > surface brightness conversions, that may need pixel scale factor?

yes, it should

where does it get that from if there is no spectrum.meta or additional equivalency?

the for-loop directly above this (it does rely on layer.layer.meta, but that should be set by spectral extraction)

On L74 there is a loop to find the scale factor if it exists, and if so, to attach it to the metadata of the spectral_axis. Then there is a small modification to flux_conversion to account for passing the scale factor to spectral_axis instead of spec(for the case that spec nor eqv is not passed).

thanks, i see what's happening now!

cshanahan1

I tested this out and could not break it with any unit conversion combo! I would add some tests (maybe in test_user_api) to test this against different unit conversions and translations and make sure zoom level stays preserved.

gibsongreen

Added test coverages looks good, I retested with the re-introduction of the delayed-callback and glad to see all behavior is working as expected!

kecnry added this to the 4.1 milestone Dec 3, 2024

kecnry commented Dec 3, 2024

View reviewed changes

github-actions bot added specviz plugin Label for plugins common to multiple configurations labels Dec 3, 2024

gibsongreen reviewed Dec 5, 2024

View reviewed changes

kecnry commented Dec 5, 2024

View reviewed changes

gibsongreen marked this pull request as ready for review December 9, 2024 14:28

gibsongreen requested review from rosteen, javerbukh, pllim, bmorris3, haticekaratay and cshanahan1 as code owners December 9, 2024 14:28

kecnry force-pushed the unit-conversion-y-limits branch from 12c90bd to 6411bf2 Compare December 9, 2024 18:27

kecnry added 3 commits December 9, 2024 14:36

remove call to reset limits during unit conversion

b112e27

custom logic for mapping y-limits on unit change

cf29bf3

remove delay_callback

57f892c

kecnry force-pushed the unit-conversion-y-limits branch from 5a1b45b to 701ba53 Compare December 9, 2024 19:38

changelog

b3e3ac3

kecnry force-pushed the unit-conversion-y-limits branch from 701ba53 to b3e3ac3 Compare December 9, 2024 19:48

gibsongreen and others added 5 commits December 10, 2024 08:36

avoid creating Spectrum1D object

40b4630

change slice -> spectral_axis, checks for len and type

9d0124d

fix flux_conversion condition, update paramater names, add exceptions

c91ea9b

update test arguments

b1e3657

reintroduce Quantity, add scale factor handling

81f6b97

kecnry force-pushed the unit-conversion-y-limits branch from 3cec14b to 81f6b97 Compare December 10, 2024 13:39

kecnry added 2 commits December 10, 2024 08:40

break out of for-loop

3e99e42

ensure info.meta is populated

031ce2c

kecnry force-pushed the unit-conversion-y-limits branch from 152e4a9 to 5e8f298 Compare December 10, 2024 14:59

handle when meta not defined

f657a92

kecnry force-pushed the unit-conversion-y-limits branch from 5e8f298 to f657a92 Compare December 10, 2024 16:46

add comment in code to generalize in the future

398aecd

astrofrog reviewed Dec 12, 2024

View reviewed changes

cshanahan1 reviewed Dec 13, 2024

View reviewed changes

cshanahan1 requested changes Dec 13, 2024

View reviewed changes

kecnry force-pushed the unit-conversion-y-limits branch from 5b59941 to 2e3483c Compare December 13, 2024 18:16

kecnry added 2 commits December 18, 2024 08:56

test coverage

bcdcf85

re-introduce delay_callback

655cd96

kecnry force-pushed the unit-conversion-y-limits branch from 7e297dd to 655cd96 Compare December 18, 2024 13:57

kecnry requested review from gibsongreen and cshanahan1 December 18, 2024 14:21

gibsongreen approved these changes Dec 19, 2024

View reviewed changes

cshanahan1 approved these changes Dec 20, 2024

View reviewed changes

kecnry merged commit 57848c2 into spacetelescope:main Dec 20, 2024
17 checks passed

kecnry deleted the unit-conversion-y-limits branch December 20, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert limits on spectrum y-unit change #3335

convert limits on spectrum y-unit change #3335

kecnry commented Dec 3, 2024 •

edited

Loading

kecnry Dec 3, 2024

kecnry Dec 13, 2024

gibsongreen Dec 5, 2024

kecnry Dec 5, 2024

gibsongreen Dec 9, 2024

gibsongreen commented Dec 5, 2024

kecnry Dec 5, 2024

gibsongreen Dec 5, 2024

codecov bot commented Dec 10, 2024 •

edited

Loading

astrofrog left a comment

astrofrog commented Dec 12, 2024

kecnry commented Dec 13, 2024

cshanahan1 Dec 13, 2024

kecnry Dec 13, 2024

cshanahan1 Dec 13, 2024

kecnry Dec 13, 2024

gibsongreen Dec 13, 2024 •

edited

Loading

cshanahan1 Dec 13, 2024

cshanahan1 left a comment

gibsongreen left a comment

		spec = Spectrum1D(spectral_axis=x_corners * u.Unit(self.x_display_unit),
		flux=y_corners * u.Unit(old_unit))

convert limits on spectrum y-unit change #3335

convert limits on spectrum y-unit change #3335

Conversation

kecnry commented Dec 3, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

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

gibsongreen commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 10, 2024 • edited Loading

Codecov Report

astrofrog left a comment

Choose a reason for hiding this comment

astrofrog commented Dec 12, 2024

kecnry commented Dec 13, 2024

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

gibsongreen Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshanahan1 left a comment

Choose a reason for hiding this comment

gibsongreen left a comment

Choose a reason for hiding this comment

kecnry commented Dec 3, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

gibsongreen Dec 13, 2024 •

edited

Loading