expanded support of SB/Flux translations #2940

gibsongreen · 2024-07-08T16:50:44Z

Description

This pull request is to address separating flux_or_sb_unit into flux_unit or sb_unit. Four total drop downs are now present in the Unit Conversion plugin (spectral unit, flux or surface brightness, flux unit, surface brightness unit).

Some units that are included in main for surface brightness/flux are incapable of being translated, and fail silently. These units have either been removed, or if one of these flux units is selected, conversion is capable within the same physical type, but translation is disabled until the unit is converted to a unit that is translatable.

Screen.Recording.2024-07-08.at.12.52.58.PM.mov

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

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

…rt, styling changes

to not require spectrum, so nothing to centralize now. Fix devdeps Simplify flux_conversion logic and other minor fixes. Add flux conversion tests. Clarify which one should be reverted in the future

because moment map always in SB

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

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

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

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

jdaviz/utils.py

kecnry

Are all the calls to reset_limits still needed (I thought the limits were now being translated correctly)?

gibsongreen · 2024-07-17T02:38:31Z

Are all the calls to reset_limits still needed (I thought the limits were now being translated correctly)?

Translating eV / (Hz s m2) and converting the untranslatable_unitsare two cases I see that still require reset_limits

codecov · 2024-07-17T19:31:37Z

Codecov Report

Attention: Patch coverage is 89.94083% with 17 lines in your changes missing coverage. Please review.

Project coverage is 88.78%. Comparing base (22ed4cf) to head (069dbbc).
Report is 144 commits behind head on main.

Files with missing lines	Patch %	Lines
...specviz/plugins/unit_conversion/unit_conversion.py	90.09%	11 Missing ⚠️
jdaviz/core/marks.py	25.00%	3 Missing ⚠️
jdaviz/core/validunits.py	85.71%	2 Missing ⚠️
...igs/specviz/plugins/line_analysis/line_analysis.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2940      +/-   ##
==========================================
- Coverage   88.78%   88.78%   -0.01%     
==========================================
  Files         111      111              
  Lines       17276    17372      +96     
==========================================
+ Hits        15338    15423      +85     
- Misses       1938     1949      +11

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

kecnry

Just a few more comments and then I think this can go in and we can iterate as needed to build on top of it. Thanks for working through all of this!

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

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

haticekaratay · 2024-07-19T13:11:06Z

After testing some more, I noticed that if we ever select the ST from the flux units, conversion from ST to any other flux seems to fail.

gibsongreen · 2024-07-19T13:41:20Z

After testing some more, I noticed that if we ever select the ST from the flux units, conversion from ST to any other flux seems to fail.

I can't reproduce this, can you send me the traceback and I can track down where that is coming from

haticekaratay · 2024-07-19T13:55:33Z

After testing some more, I noticed that if we ever select the ST from the flux units, conversion from ST to any other flux seems to fail.

I can't reproduce this, can you send me the traceback and I can track down where that is coming from

It had to do with my env. I don't see it anymore after re-installing the branch. All good.

pllim

Here is technical review. I will do functional review next week once I know what was the conclusion from the 1PM meeting today with Cami w.r.t. UI changes. Thanks!

jdaviz/app.py

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

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

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

jdaviz/core/marks.py

jdaviz/core/validunits.py

jdaviz/tests/test_utils.py

pllim

Here is the rest of technical review since Kyle says I should unskip some files. 😆

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

pllim · 2024-07-22T20:52:12Z

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

+            self.app.data_collection[0] and
+            self.app.config == 'specviz'
+        ):
+            if check_if_unit_is_per_solid_angle(self.app.data_collection[0].get_object()._unit):


Are we confident that get_object() would always return an object with _unit attribute for the first loaded data? What assumption are we making here?

With all the talks about "free-form," are we shooting ourselves in our future foot?

If there is a safer way to attain the units from the data item that keeps us more open to enable free-form in the future .

Accessing a hidden _unit (leading underscore usually hints that it is private and should not be used externally). I think you are basically trying to find flux unit, so any reason to not use a more public get_object().flux.unit (since this is limited to specviz so you are getting Spectrum1D back). Where did _unit come from anyway?

Looks like it is buried within specutils class mixin. @rosteen , what is the difference between Spectrum1D._unit and Spectrum1D.flux.unit?

It's not from the specutils class mixin as far as I can tell, I suspect it's inherited from a couple layers upstream. I'd just use flux.unit.

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

pllim · 2024-07-22T20:56:41Z

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

+
+        yunit = _valid_glue_display_unit(flux_or_sb, self.spectrum_viewer, 'y')
+        # may need to be updated if translations in other configs going to be supported
+        if not self.flux_unit.choices and self.app.config == 'cubeviz':


Should this check happen much earlier in this method to avoid unnecessary computations?

I don't see why it can't happen earlier, let me test it out earlier in the _on_flux_unit_changed and I'll make the change bearing no behavioral change occurs!

I do get an AttributeError: 'UnitConversion' object has no attribute 'flux_unit' if I move it further up. I can spend some time seeing if/what I can change the conditional to to have the same behavior.

you can add a if not hasattr(self, 'flux_unit'): return to the top of the method if you'd like. The observe on flux_unit_selected can get triggered when the traitlet is created but before the plugin's init has created the flux_unit object itself, and there's no need to run all this logic at that point.

Does this mean self.flux_unit.choices changes all the time throughout the logic in this method?

I thought I had tried the not hasattr before, but irregardless that seemed to do the trick and it is now at the top of the function. Just running through tests locally now to make sure it.

The flux_unit can be created, but the self.flux_unit.choices isn't populated yet causing one of the AttributeErrors, but the actually choices don't change once populated.

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

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

pllim · 2024-07-22T22:07:33Z

In terms of functionality (part 1: cubeviz):

What does erg / (s cm2) really mean here in the flux unit drop-down? Don't you only get that after integrating over some wavelength/frequency?
Does AB/sr look right? It is in the order of 1E+7 in the example Cubeviz notebook data (see screenshot below). Do people really do science in this unit? Maybe a question for Patrick and Cami.
Creating a new spectral subset (after having 2 spatial subset) is noticeably slow. I don't know if it is caused by all the math in this PR or what. I feel like we had seen this before and Jesse had fixed it some time ago but now it's back.
As a user, it is unclear to me if the fitted model parameter values are in native data unit or the unit I picked, and if the latter, whether it is "flux" or "surface brightness". I just see a table with raw numbers.
Moment Map switching between flux/SB -- Is that a follow-up ticket? Kyle said it is supposed to be always in native data unit, right?
Just to confirm, none of these new stuff affect aperture photometry, right, because it already does the PIXAR_SR stuff behind the scenes even before Unit Conversion existed? From the diff, I don't think so but I also have not be following all the other PRs.

pllim · 2024-07-23T01:28:30Z

In terms of functionality (part 2: otherviz):

Skipped Imviz, Mosviz, Specviz2d because they have no unit conversion.
specviz has Unit Conversion plugin but no toggle between flux and SB.

kecnry · 2024-07-23T12:09:43Z

Creating a new spectral subset (after having 2 spatial subset) is noticeably slow. I don't know if it is caused by all the math in this PR or what. I feel like we had seen this before and Jesse had fixed it some time ago but now it's back.

Can you check on main and if its there as well, file a tech debt ticket please?

As a user, it is unclear to me if the fitted model parameter values are in native data unit or the unit I picked, and if the latter, whether it is "flux" or "surface brightness". I just see a table with raw numbers.
Moment Map switching between flux/SB -- Is that a follow-up ticket? Kyle said it is supposed to be always in native data unit, right?
Just to confirm, none of these new stuff affect aperture photometry, right, because it already does the PIXAR_SR stuff behind the scenes even before Unit Conversion existed? From the diff, I don't think so but I also have not be following all the other PRs.
Skipped Imviz, Mosviz, Specviz2d because they have no unit conversion.

All of these are separate follow-up tickets already discussed and assigned. For aperture photometry, this will be limited to the results table displaying in the user-preferred units, with no need to convert SB<>flux.

specviz has Unit Conversion plugin but no toggle between flux and SB.

This is by design - afaik, we don't have access to PIXAR_SR in pipeline extracted spectra.

camipacifici · 2024-07-23T13:43:17Z

This is by design - afaik, we don't have access to PIXAR_SR in pipeline extracted spectra.

The 1D spectra from JWST come with 2 columns for flux, one in surface brightness and one in flux (I do not remember if this depends of the source being defined as point or extended). So technically, specviz has all the information to display one or the other. I will double check what it is actually loading...I do not remember anymore.

pllim · 2024-07-23T13:54:05Z

@camipacifici , it is here: https://github.com/astropy/specutils/blob/a6fd06f975306b1c7a15337461e1e3873fe178a7/specutils/io/default_loaders/jwst_reader.py#L350-L360

Looks like specutils made the choice for you.

camipacifici · 2024-07-23T13:57:01Z

That's what I thought, thank you!
As a follow up, maybe we should give the user the choice to load flux or surface brightness. They should both be available in each x1d file. What do you think?

rosteen · 2024-07-23T13:58:57Z

There was already a request in specutils to make the choice at load time in Spectrum1D.read() rather than assuming based on point source vs extended, I could prioritize making that change if we also want to use it here.

kecnry · 2024-07-23T13:59:05Z

If we load both into glue (when loading from a file), we could easily swap out the columns... but we'll still need to handle detecting when we can and cannot do that. Perhaps an idea for future expansion?

pllim · 2024-07-23T14:32:43Z

Can you check on main and if its there as well, file a tech debt ticket please?

Performance seems normal on main.

kecnry · 2024-07-23T14:35:14Z

Can you give steps to reproduce? Is this only after changing the display units? That could be something to report upstream in glue if the units of the subset need to be mapped to the viewer or something.

pllim · 2024-07-23T16:03:16Z

Can you give steps to reproduce?

Starts Cubeviz example notebook. Load the data.
In Unit Conversion plugin, change to Surface Brightness.
Draw circle region around star. Draw rectangle region on a background area. See 3 spectra in spectrum viewer when you reset its zoom.
Draw one spectral region over the spectra. The lag is a heartbeat. Slower than normal but only if you are paying attention.
Draw second spectral region somewhere else. The lag is now half a sec before things are finalized.

If no one else sees this, then never mind.

kecnry · 2024-07-23T16:50:54Z

I can't reproduce anything significant. Do you see the same thing on main when switching from wavelength to frequency for the spectral axis? If so, I think we can say this PR isn't to blame and investigate separately.

pllim

LGTM. Since Cami already has performance as a broad goal at some point, we can defer the lag I saw.

gibsongreen added cubeviz plugin Label for plugins common to multiple configurations labels Jul 8, 2024

gibsongreen added this to the 4.0 milestone Jul 8, 2024

github-actions bot added the specviz label Jul 8, 2024

kecnry reviewed Jul 9, 2024

View reviewed changes

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

gibsongreen force-pushed the flux_sb_all branch from a1350ef to 2146060 Compare July 10, 2024 14:21

gibsongreen and others added 9 commits July 12, 2024 09:49

UI functioning unit conversion, need to reconcile tests

8f99b93

update syntax, remove unnecessary logic

a1e0b11

tests now pass, API functionality improved

6e32a93

add additional equivalencies for coords info

3013fb8

first pass at disabling specviz translations

69cce01

Remove dev flag, consolidate untranslatable units, multi-config suppo…

87ca242

…rt, styling changes

Refactor moment map unit conversion

24ff03a

to not require spectrum, so nothing to centralize now. Fix devdeps Simplify flux_conversion logic and other minor fixes. Add flux conversion tests. Clarify which one should be reverted in the future

Remove SB/flux conversion for moment map

59eb9f2

because moment map always in SB

add os import

d7686f4

gibsongreen force-pushed the flux_sb_all branch from ee92756 to d7686f4 Compare July 12, 2024 13:53

remove duplicate import from rebase

e99058a

kecnry reviewed Jul 12, 2024

View reviewed changes

reconcile tests, change hints/docs, handle dimensionless case

fa7c9a5

kecnry reviewed Jul 15, 2024

View reviewed changes

emit GlobalDisplayUnitChanged msg when toggling SB<>flux

da0a967

kecnry force-pushed the flux_sb_all branch from 4c3591c to da0a967 Compare July 15, 2024 19:40

resolve most tests

0c96e2e

gibsongreen added 3 commits July 17, 2024 10:23

temporarily remove translation message

72295cf

add comments, reenable global display message, resolve tests

aa718e3

force sb unit for line flux test

f5b1f48

gibsongreen marked this pull request as ready for review July 17, 2024 19:45

gibsongreen requested a review from rosteen as a code owner July 17, 2024 19:45

kecnry reviewed Jul 19, 2024

View reviewed changes

use astropy units, small logic tweaks, cache untranslatable units

c385aff

pllim reviewed Jul 19, 2024

View reviewed changes

pllim mentioned this pull request Jul 19, 2024

Investigate moment map commented test #2936

Closed

gibsongreen added 2 commits July 22, 2024 10:22

technical review changes

7be903f

add warning message in tray if PIXAR_SR not present in FITS header

b46c001

pllim reviewed Jul 22, 2024

View reviewed changes

gibsongreen changed the title ~~SB/Flux conversion across all configs~~ expanded support of SB/Flux translations Jul 23, 2024

second pass at technical changes

6eb01c0

Merge remote-tracking branch 'upstream/main' into flux_sb_all

dffa678

gibsongreen added 2 commits July 23, 2024 12:59

move flux_unit checks to beginning of _on_flux_unit_changed

9067ef5

change _unit to flux.unit

069dbbc

pllim approved these changes Jul 23, 2024

View reviewed changes

pllim merged commit 143cd8d into spacetelescope:main Jul 23, 2024
16 of 19 checks passed

gibsongreen mentioned this pull request Jul 30, 2024

Surface brightness -> solid angle conversion updates #3111

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expanded support of SB/Flux translations #2940

expanded support of SB/Flux translations #2940

gibsongreen commented Jul 8, 2024 •

edited

Loading

kecnry left a comment

gibsongreen commented Jul 17, 2024

codecov bot commented Jul 17, 2024 •

edited

Loading

kecnry left a comment

haticekaratay commented Jul 19, 2024

gibsongreen commented Jul 19, 2024

haticekaratay commented Jul 19, 2024 •

edited

Loading

pllim left a comment

pllim left a comment

pllim Jul 22, 2024

gibsongreen Jul 23, 2024

pllim Jul 23, 2024

pllim Jul 23, 2024

rosteen Jul 23, 2024

pllim Jul 22, 2024

gibsongreen Jul 23, 2024

gibsongreen Jul 23, 2024

kecnry Jul 23, 2024

pllim Jul 23, 2024

gibsongreen Jul 23, 2024

gibsongreen Jul 23, 2024

pllim commented Jul 22, 2024

pllim commented Jul 23, 2024

kecnry commented Jul 23, 2024

camipacifici commented Jul 23, 2024

pllim commented Jul 23, 2024

camipacifici commented Jul 23, 2024

rosteen commented Jul 23, 2024

kecnry commented Jul 23, 2024

pllim commented Jul 23, 2024

kecnry commented Jul 23, 2024

pllim commented Jul 23, 2024

kecnry commented Jul 23, 2024

pllim left a comment

expanded support of SB/Flux translations #2940

expanded support of SB/Flux translations #2940

Conversation

gibsongreen commented Jul 8, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

kecnry left a comment

Choose a reason for hiding this comment

gibsongreen commented Jul 17, 2024

codecov bot commented Jul 17, 2024 • edited Loading

Codecov Report

kecnry left a comment

Choose a reason for hiding this comment

haticekaratay commented Jul 19, 2024

gibsongreen commented Jul 19, 2024

haticekaratay commented Jul 19, 2024 • edited Loading

pllim left a comment

Choose a reason for hiding this comment

pllim 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

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

pllim commented Jul 22, 2024

pllim commented Jul 23, 2024

kecnry commented Jul 23, 2024

camipacifici commented Jul 23, 2024

pllim commented Jul 23, 2024

camipacifici commented Jul 23, 2024

rosteen commented Jul 23, 2024

kecnry commented Jul 23, 2024

pllim commented Jul 23, 2024

kecnry commented Jul 23, 2024

pllim commented Jul 23, 2024

kecnry commented Jul 23, 2024

pllim left a comment

Choose a reason for hiding this comment

gibsongreen commented Jul 8, 2024 •

edited

Loading

codecov bot commented Jul 17, 2024 •

edited

Loading

haticekaratay commented Jul 19, 2024 •

edited

Loading