refactor unit conversion messaging #3192

kecnry · 2024-09-12T15:35:17Z

Description

This pull request refactors and simplifies the messaging within unit conversion. ~~This currently is branched off of #3156, so should be moved onto main after that is merged.~~

TODO

Unskip test_spectrum_viewer_axis_labels from convert flux cubes to per-square-pixel surface brightness cubes #3156
Consolidate PIX2 vs u.pix * u.pix from convert flux cubes to per-square-pixel surface brightness cubes #3156 - defer for follow-up by @cshanahan1

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

pllim

Don't grok most of the stuff. Just a general review. Thanks.

pllim · 2024-09-17T15:27:37Z

jdaviz/app.py

@@ -86,7 +86,16 @@ def equivalent_units(self, data, cid, units):
 'erg / (s sr cm2)', 'erg / (Hz s sr cm2)',
 'erg / (Angstrom s sr cm2)',
 'ph / (Angstrom s sr cm2)', 'ph / (Hz s sr cm2)'
- ])
+ ]
+ + [


Do we really need the + operators to chain multiple static lists here? This introduces unnecessary list math, no?

jdaviz/configs/cubeviz/plugins/parsers.py

jdaviz/configs/cubeviz/plugins/tests/test_cubeviz_aperphot.py

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

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

pllim · 2024-09-17T15:50:38Z

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

+ # If unit is flux per pix2, the type will be 'unknown' rather
+ # than surface brightness, so have to multiply the pix2 part out
+ # and check if the numerator is a spectral flux density
+ if check_if_unit_is_per_solid_angle(unit, return_unit=True) == u.pix*u.pix:


Should we move PIX2 higher up and re-use it throughout the package instead of u.pix*u.pix?

I'll do this after all these PRs are merged to avoid conflicts

jdaviz/tests/test_utils.py

jdaviz/utils.py

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

cshanahan1 · 2024-09-22T01:20:40Z

jdaviz/configs/cubeviz/plugins/mixins.py

@@ -89,6 +89,12 @@ def slice_values(self):
 take_inds = [2, 1, 0]
 take_inds.remove(self.slice_index)
 converted_axis = np.array([])
+
+ # Retrieve display units


just confirming, this was moved because the slice display units don't depend on layer?

right, I moved this above the for-loop since it isn't needed to be called repeatedly

cshanahan1

looks good! left a few comments. This is a great improvement that makes it a lot clearer when unit changing messages are sent.

Could you explain briefly what the changes to slice are doing?

jdaviz/configs/default/plugins/viewers.py

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

kecnry · 2024-09-23T12:01:14Z

Could you explain briefly what the changes to slice are doing?

Sure! Changes to slice do the following:

handle the changed order in which the unit gets populated by not even attempting to pull the slice values from a viewer without set units
improved cache management to insure that slices are retrieved in the correct units
future-proof by separating the unit conversion into its own method so it can be overridden when different equivalencies are needed. This might eventually want to call the more general shared-logic for handling unit conversion app-wide.

kecnry · 2024-09-26T13:21:18Z

re lag - there actually isn't more lag (as far as I can tell), but rather the attribute display limits are being updated before the spectral y units, and since the y units are what is the most visible immediate change, it feels like more lag. I'm working to try to optimize setting the display limits, since deferring changing those until last would require additional muddy logic.

kecnry · 2024-09-26T13:28:58Z

I was actually able to get the y-unit to be updated first and optimize attribute_display_unit - hopefully tests still pass 🤞. Let me know if you still see any noticeable difference in responsiveness between this and main.

This reverts commit aa0d384.

* apply handle_spectral_y_unit before attribute_display_unit * optimize handle_attribute_display_unit by caching image layers

cshanahan1

Tested this again, the issues I commented yesterday are resolved, looks good!

gibsongreen

I went through unit_conversion.py one more time and generally retested along with the comments from yesterday. Everything looks good to me, nice work!

kecnry added this to the 4.0 milestone Sep 12, 2024

github-actions bot added cubeviz specviz testing imviz plugin Label for plugins common to multiple configurations labels Sep 12, 2024

kecnry force-pushed the unit-conv-messaging branch from 4cfaeed to 9293060 Compare September 12, 2024 15:43

kecnry changed the title ~~refactor unit conv messaging~~ refactor unit conversion messaging Sep 12, 2024

kecnry force-pushed the unit-conv-messaging branch from 55c36a8 to 18e1562 Compare September 12, 2024 17:32

camipacifici mentioned this pull request Sep 12, 2024

convert flux cubes to per-square-pixel surface brightness cubes #3156

Merged

kecnry force-pushed the unit-conv-messaging branch 8 times, most recently from 405a11d to cee44db Compare September 16, 2024 15:43

pllim reviewed Sep 17, 2024

View reviewed changes

jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py Outdated Show resolved Hide resolved

kecnry mentioned this pull request Sep 18, 2024

fix formatting of unit after appending angle #3198

Closed

kecnry force-pushed the unit-conv-messaging branch 4 times, most recently from d54cef4 to 9d04c43 Compare September 20, 2024 19:52

cshanahan1 reviewed Sep 22, 2024

View reviewed changes

jdaviz/configs/default/plugins/viewers.py Show resolved Hide resolved

jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py Outdated Show resolved Hide resolved

kecnry force-pushed the unit-conv-messaging branch 2 times, most recently from 235fe11 to 23fcaa6 Compare September 24, 2024 15:18

kecnry added 22 commits September 26, 2024 13:19

WIP: refactor/simplify messaging and logic in unit conversion

0abc22b

generalized solution in place of spacetelescope#3185

452eb5e

code cleanup

a1b5bae

generalized solution in place of spacetelescope#3177

5bd8241

update slice plugin to handle new messaging order

186868a

generalize initializing options to also act on first valid data

58e9d63

code cleanup

9607eb4

fix test failures

cc43f45

Revert "skip test in parsers"

72b4677

This reverts commit aa0d384.

fix rebase failure

08cb09c

unskip test

029955c

replace time axis elif with comment

8fb3d5d

add e/s

7a2fef7

changelog entry

51e2f70

revert debugging separation

08611aa

fix ylabel test failures

4fc5459

code simplification

5ea0327

fix model_fitting test failures

54c65df

ignore multiple slashes warning

721ac05

fix uncertainty unit conversion

b356b59

re-introduce resetting y-limits (until can be fixed)

59d1192

optimize changing spectral y-unit

d467ccb

* apply handle_spectral_y_unit before attribute_display_unit * optimize handle_attribute_display_unit by caching image layers

kecnry force-pushed the unit-conv-messaging branch from fd6562b to d467ccb Compare September 26, 2024 17:19

cshanahan1 approved these changes Sep 26, 2024

View reviewed changes

kecnry added the Ready for final review label Sep 26, 2024

gibsongreen approved these changes Sep 26, 2024

View reviewed changes

kecnry merged commit c9425b8 into spacetelescope:main Sep 26, 2024
25 checks passed

kecnry deleted the unit-conv-messaging branch September 26, 2024 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor unit conversion messaging #3192

refactor unit conversion messaging #3192

kecnry commented Sep 12, 2024 •

edited

Loading

pllim left a comment

pllim Sep 17, 2024

pllim Sep 17, 2024

cshanahan1 Sep 18, 2024

cshanahan1 Sep 22, 2024

kecnry Sep 23, 2024

cshanahan1 left a comment •

edited

Loading

kecnry commented Sep 23, 2024

kecnry commented Sep 26, 2024

kecnry commented Sep 26, 2024

cshanahan1 left a comment

gibsongreen left a comment

refactor unit conversion messaging #3192

refactor unit conversion messaging #3192

Conversation

kecnry commented Sep 12, 2024 • edited Loading

Description

TODO

Change log entry

Checklist for package maintainer(s)

pllim left a comment

Choose a reason for hiding this comment

pllim Sep 17, 2024

Choose a reason for hiding this comment

pllim Sep 17, 2024

Choose a reason for hiding this comment

cshanahan1 Sep 18, 2024

Choose a reason for hiding this comment

cshanahan1 Sep 22, 2024

Choose a reason for hiding this comment

kecnry Sep 23, 2024

Choose a reason for hiding this comment

cshanahan1 left a comment • edited Loading

Choose a reason for hiding this comment

kecnry commented Sep 23, 2024

kecnry commented Sep 26, 2024

kecnry commented Sep 26, 2024

cshanahan1 left a comment

Choose a reason for hiding this comment

gibsongreen left a comment

Choose a reason for hiding this comment

kecnry commented Sep 12, 2024 •

edited

Loading

cshanahan1 left a comment •

edited

Loading