Enable spectral conversion in cubeviz #2758

javerbukh · 2024-03-15T14:39:02Z

Description

This pull request enables unit conversion in cubeviz only for spectral conversions. The following plugins also need to be updated to account for possible spectral axis unit changes:

Potential follow-up efforts:

Fix moment map (with continuum equal to anything but None) not working with frequency units
Fix slice indicator disappearing after unit change

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.
Enable spectral unit conversion in cubeviz and update plugins [2758]

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

rosteen

I found two things in initial testing:

The moment map plugin still breaks if you've changed spectral units and use "Surrounding" for the continuum, looks like it's getting an empty array for the continuum due to the differing units.
We should probably also change the units shown in the Spectral Extraction plugin if the display units have changed. The plugin still works as-is, but the wavelength is in the original units.

I also left one comment on the code.

rosteen · 2024-03-15T16:14:50Z

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

        xunit = _valid_glue_display_unit(self.spectral_unit.selected, self.spectrum_viewer, 'x')
        if self.spectrum_viewer.state.x_display_unit != xunit:
            self.spectrum_viewer.state.x_display_unit = xunit
+        self.hub.broadcast(GlobalDisplayUnitChanged('spectral',


Can these messages actually go inside the if block? We don't need to broadcast this if the unit didn't actually change, do we?

What was the reason for changing the order? I'm trying to think if there are any (unintentional) consequences to that... but hopefully it is fine if its needed for something else.

I was using the spectrum_viewer to get the spectral units while developing so I moved it to after that change. Now that it is in the if statement I don't know if it matters anymore.

pllim · 2024-03-15T17:38:44Z

How does/will the "per sterdian" thingy fit into this?

kecnry · 2024-03-19T17:53:30Z

How does/will the "per sterdian" thingy fit into this?

This is to prepare for that by bringing the plugin in and enable conversion to the spectral axis, intentionally deferring flux so that we can decide whether to lump flux and surface brightness together or have as separate dropdowns, etc.

codecov · 2024-03-19T19:24:32Z

Codecov Report

Attention: Patch coverage is 98.68421% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.79%. Comparing base (f1ba4a6) to head (7339694).
Report is 29 commits behind head on main.

❗ Current head 7339694 differs from pull request most recent head 3c1641c. Consider uploading reports for the commit 3c1641c to get more accurate results

Files	Patch %	Lines
jdaviz/configs/cubeviz/plugins/viewers.py	97.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2758      +/-   ##
==========================================
+ Coverage   88.72%   88.79%   +0.07%     
==========================================
  Files         110      110              
  Lines       16353    16607     +254     
==========================================
+ Hits        14509    14746     +237     
- Misses       1844     1861      +17

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

javerbukh · 2024-03-19T20:02:16Z

@rosteen @gibsongreen The problems Ricky was seeing should be fixed, please re-review when you can.

rosteen · 2024-03-19T20:38:32Z

The moment map calculation looks fixed, but this still has some broken features in my testing. The unit shown in the aperture photometry is correct, but the numerical value still looks wrong:

I think that's due to the fact that after doing unit conversion, I'm unable to select a slice in the new wavelength range (it always selects the max available value for the old unit in my case, since 7000 > 7.65 in the um -> nm case). So spectral extraction and aperture photometry are stuck thinking the slice is 7.65 nm. This also affects the wavelength shown in the spectral extraction plugin when you select Wavelength dependent.

javerbukh · 2024-03-25T13:38:02Z

@rosteen Seems like if you avoid using model fitting, everything else works with spectral unit conversion. Any ideas why that might be happening?

Edit: it does work in specviz, just not cubeviz.
2nd edit: might've figured it out, will update.

javerbukh · 2024-03-26T17:51:42Z

jdaviz/configs/cubeviz/plugins/viewers.py

+                spec_axis = layer.layer.data.get_object(cls=Spectrum1D).spectral_axis
+                display_spectral_units = self.jdaviz_app._get_display_unit('spectral')
+                if display_spectral_units is not '' and spec_axis.unit != display_spectral_units:
+                    converted_axis = spec_axis.to(display_spectral_units).value
+                else:
+                    converted_axis = spec_axis.value


This is making slice selection much slower but I do not have a great solution for this at the moment.

I was hoping that it only slowed down slice selection after the spectral axis had been converted, but it does seem a little bit slower right from the start as well. Probably not a deal-breaker but a little frustrating.

can we just cache the scale factor between native and display units so that we don't need to do units math here repeatedly?

rosteen · 2024-03-26T19:01:20Z

It looks like the bugs I found are fixed, but the test failures look real. One other thing - although slicing works now, when you change units the slice indicator does still end up off the screen initially. Maybe we can set the slice indicator to the same pixel index after changing units? Looking much closer to ready to merge!

jdaviz/configs/cubeviz/plugins/slice/slice.py

jdaviz/configs/cubeviz/plugins/slice/tests/test_slice.py

jdaviz/configs/cubeviz/plugins/viewers.py

kecnry · 2024-04-02T20:29:27Z

Should we remilestone this to 3.10 just so we don't accidentally merge this shortly before the 3.9 release and we don't get a chance to test it properly?

kecnry · 2024-04-03T12:36:48Z

jdaviz/configs/cubeviz/plugins/viewers.py

+        del self.slice_values
+        self.slice_values


This would really benefit from #2253, but for now, should be:

Suggested change

del self.slice_values

self.slice_values

if 'slice_values' in self.__dict__:

del self.__dict__['slice_values']

(and same for the other case)

Add message to flux conversion dropdown in cubeviz Have moment map units update after conversion Update continuum, moment maps, and spectral extraction Fix model fitting and aperture plugins and add tests Update slice_values to account for unit conversion Remove print statment and unused import Fix style Fix moment map and slice tests Fix style 2 Add LinkSameWithUnits and slice indicator updates

… twice

rosteen · 2024-04-11T19:09:01Z

I just pushed a commit to fix the slice indicator behavior after unit conversion. Basically, the indicator mark and the slice plugin itself both had logic to convert to the new unit, but neither knew that it had already been done by the other so it was happening twice. My commit overrides the method inherited from the base PluginMark to only update the unit string, letting the plugin handle updating the actual value.

rosteen

I might have a conflict of interest now that I committed code, but I think this is good to go at this point.

gibsongreen

Awesome to see this in and performant, I'm excited to rebase and start trying spectral/flux/sb conversions all together. Great work!

jdaviz/configs/cubeviz/plugins/viewers.py

jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.vue

pllim · 2024-04-12T17:42:51Z

jdaviz/core/template_mixin.py

@@ -2871,8 +2872,8 @@ def _get_continuum(self, dataset, spatial_subset, spectral_subset, update_marks=
                          'center': spectral_axis.value,
                          'right': []}
            else:
-                mark_x = {'left': spectral_axis_nanmasked[spectral_axis.value < sr_lower.value],
-                          'right': spectral_axis_nanmasked[spectral_axis.value > sr_upper.value]}
+                mark_x = {'left': spectral_axis_nanmasked[spectral_axis < sr_lower],


I am unable to tell from a quick glance if mark_x is going into any front-end message. Looks like you are now passing Quantity instead of float arrays? Is the front-end JSON thingy able to handle this without warning?

From my testing it is, did you experience a bug relating to this?

In the past, I had seen warnings polluting the notebook cell outputs when JSON is trying to serialize something fancy like Quantity. But if you don't see it, then maybe it is not a problem here. Just double checking. Thanks!

The quantity is just being used to determine the indices though, right? spectral_axis_nanmasked is unchanged here.

javerbukh · 2024-04-12T18:18:46Z

Thank you for the reviews @kecnry and @pllim, please let me know if your comments were adequately addressed.

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

kecnry · 2024-04-15T12:41:16Z

jdaviz/configs/cubeviz/plugins/viewers.py

+    @property
+    def slice_display_unit_name(self):
+        return 'spectral'


same thing here (can we rename to slice_axis or rename slice_axis in slice to this?)

I changed slice_axis to slice_display_unit_name, let me know if another name sounds more appropriate. Otherwise I can merge once CI passes (whatever can run right now at least).

kecnry

I was now able to write the overrides in lcviz to handle these changes - so thanks for your patience with all my requests (/demands) to try to generalize this so much!

I do think a rename of the property to be consistent between here and the slice plugin would help make it less confusing internally in the future, but otherwise I think this is good to go and any further generalization can be handled as needed when actually integrating into lcviz. Thanks!

* Add unit conversion plugin with flux conversion disabled Add message to flux conversion dropdown in cubeviz Have moment map units update after conversion Update continuum, moment maps, and spectral extraction Fix model fitting and aperture plugins and add tests Update slice_values to account for unit conversion Remove print statment and unused import Fix style Fix moment map and slice tests Fix style 2 Add LinkSameWithUnits and slice indicator updates * Use cached_property for slice_values * Use correct cache clearing protocol Running CI without cached_property Fix everything except todo statements Uncommented code needed for uncert viewer to change slice Remove comments * Fix message handlers * Remove space * Minor changes * Override display unit handling in slice mark so we don't change value twice * Simplify and generalize viewer slice values code * Fix style * Move code out of try except block * Address review comments * Generalize display unit call in spectral extraction * Fix test failure * Rename slice_axis property to slice_display_unit_name * Add changelog --------- Co-authored-by: Ricky O'Steen <rosteen@stsci.edu>

javerbukh requested review from rosteen, pllim, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners March 15, 2024 14:39

github-actions bot added cubeviz specviz plugin Label for plugins common to multiple configurations labels Mar 15, 2024

javerbukh added this to the 3.9 milestone Mar 15, 2024

rosteen reviewed Mar 15, 2024

View reviewed changes

javerbukh force-pushed the enable-spectral-conversion-in-cubeviz branch from b94cc85 to 7e64afd Compare March 26, 2024 14:43

javerbukh commented Mar 26, 2024

View reviewed changes

kecnry reviewed Mar 26, 2024

View reviewed changes

jdaviz/configs/cubeviz/plugins/slice/slice.py Outdated Show resolved Hide resolved

kecnry reviewed Apr 2, 2024

View reviewed changes

jdaviz/configs/cubeviz/plugins/slice/tests/test_slice.py Outdated Show resolved Hide resolved

javerbukh commented Apr 2, 2024

View reviewed changes

jdaviz/configs/cubeviz/plugins/viewers.py Outdated Show resolved Hide resolved

javerbukh modified the milestones: 3.9, 3.10 Apr 2, 2024

kecnry reviewed Apr 3, 2024

View reviewed changes

javerbukh force-pushed the enable-spectral-conversion-in-cubeviz branch from a03ba2b to 268f6d2 Compare April 10, 2024 01:04

javerbukh and others added 4 commits April 11, 2024 15:07

Fix message handlers

78d4db3

Remove space

a2595d0

Minor changes

19dc4b5

Override display unit handling in slice mark so we don't change value…

bfcf69d

… twice

rosteen force-pushed the enable-spectral-conversion-in-cubeviz branch from 2214759 to bfcf69d Compare April 11, 2024 19:07

rosteen approved these changes Apr 11, 2024

View reviewed changes

gibsongreen approved these changes Apr 12, 2024

View reviewed changes

javerbukh added 2 commits April 12, 2024 11:51

Simplify and generalize viewer slice values code

12ea01b

Fix style

5e958bb

kecnry reviewed Apr 12, 2024

View reviewed changes

jdaviz/configs/cubeviz/plugins/viewers.py Outdated Show resolved Hide resolved

jdaviz/configs/cubeviz/plugins/viewers.py Outdated Show resolved Hide resolved

kecnry reviewed Apr 12, 2024

View reviewed changes

jdaviz/configs/cubeviz/plugins/viewers.py Outdated Show resolved Hide resolved

pllim reviewed Apr 12, 2024

View reviewed changes

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

pllim reviewed Apr 12, 2024

View reviewed changes

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

pllim reviewed Apr 12, 2024

View reviewed changes

javerbukh added 2 commits April 12, 2024 14:04

Move code out of try except block

f0ff664

Address review comments

49c44be

javerbukh added 2 commits April 12, 2024 14:24

Generalize display unit call in spectral extraction

e29b3b4

Fix test failure

118227a

kecnry reviewed Apr 15, 2024

View reviewed changes

jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py Show resolved Hide resolved

kecnry reviewed Apr 15, 2024

View reviewed changes

Rename slice_axis property to slice_display_unit_name

7339694

kecnry mentioned this pull request Apr 15, 2024

jdaviz 3.10 updates spacetelescope/lcviz#105

Merged

2 tasks

Add changelog

3c1641c

javerbukh mentioned this pull request Apr 15, 2024

[BUG] Moment map creates nan image with frequency units and continuum #2801

Closed

javerbukh merged commit 6c57b66 into spacetelescope:main Apr 15, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable spectral conversion in cubeviz #2758

Enable spectral conversion in cubeviz #2758

javerbukh commented Mar 15, 2024 •

edited

Loading

rosteen left a comment

rosteen Mar 15, 2024

kecnry Mar 19, 2024

javerbukh Mar 19, 2024

pllim commented Mar 15, 2024

kecnry commented Mar 19, 2024

codecov bot commented Mar 19, 2024 •

edited

Loading

javerbukh commented Mar 19, 2024

rosteen commented Mar 19, 2024

javerbukh commented Mar 25, 2024 •

edited

Loading

javerbukh Mar 26, 2024

rosteen Mar 26, 2024

kecnry Mar 27, 2024

rosteen commented Mar 26, 2024 •

edited

Loading

kecnry commented Apr 2, 2024

kecnry Apr 3, 2024

rosteen commented Apr 11, 2024

rosteen left a comment

gibsongreen left a comment

pllim Apr 12, 2024

javerbukh Apr 12, 2024

pllim Apr 12, 2024

kecnry Apr 15, 2024

javerbukh commented Apr 12, 2024

kecnry Apr 15, 2024

javerbukh Apr 15, 2024

kecnry left a comment

Enable spectral conversion in cubeviz #2758

Enable spectral conversion in cubeviz #2758

Conversation

javerbukh commented Mar 15, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

rosteen left a 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

pllim commented Mar 15, 2024

kecnry commented Mar 19, 2024

codecov bot commented Mar 19, 2024 • edited Loading

Codecov Report

javerbukh commented Mar 19, 2024

rosteen commented Mar 19, 2024

javerbukh commented Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen commented Mar 26, 2024 • edited Loading

kecnry commented Apr 2, 2024

Choose a reason for hiding this comment

rosteen commented Apr 11, 2024

rosteen left a comment

Choose a reason for hiding this comment

gibsongreen left a 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

javerbukh commented Apr 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kecnry left a comment

Choose a reason for hiding this comment

javerbukh commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 19, 2024 •

edited

Loading

javerbukh commented Mar 25, 2024 •

edited

Loading

rosteen commented Mar 26, 2024 •

edited

Loading