fix flux unit conversions in plugins #3228

cshanahan1 · 2024-10-18T12:54:48Z

Flux unit conversions done in plugins go through the 'flux_conversion_general' function that handles all equivalencies and special cases where the angle needs to be multiplied out before conversion. plugins that listen to unit conversion are tested with every combination of units to ensure all units that the spectral y axis can be translated to work in the plugin.

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

pllim · 2024-11-01T18:38:55Z

What is the warning about? Might be related.

UserWarning: Given trait value dtype "float64" does not match required type "float64". A coerced copy has been created.

pllim · 2024-11-01T18:39:27Z

Does this need a change log?

pllim · 2024-11-01T18:40:26Z

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

+                        # but I'm keeping them since they were already here. Background
+                        # should only be converted flux<>flux or sb<>sb so only a possible
+                        # u.spectral_density would be needed. explore removing these as a follow up
+                        equivs = u.spectral() +\


I think \ for continuation is discouraged in Python. Instead please use ().

pllim · 2024-11-01T18:41:16Z

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

@@ -308,7 +320,11 @@ def _get_defaults_from_metadata(self, dataset=None):
            # if display unit is different, translate
            if (self.config == 'cubeviz') and (self.display_unit != ''):
                disp_unit = u.Unit(self.display_unit)
-                mjy2abmag = (mjy2abmag * u.Unit("MJy/sr")).to_value(disp_unit)
+                mjy2abmag = flux_conversion_general(mjy2abmag,
+                                                    u.Unit("MJy/sr"),


This would save the overhead of string parsing.

Suggested change

u.Unit("MJy/sr"),

u.MJy / u.sr,

pllim · 2024-11-01T18:43:16Z

jdaviz/utils.py

+    Note: This is essentially a simplified version of the function utils.flux_conversion.
+    The difference is that all required equivalencies must be passed in (rather than being
+    generated from an input spectrum or slice value, and combined with input equivalences).
+    I didn't want to replace calls to that function entirely because there is some extra
+    logic there that i'm unsure of, but in theory we could/should.


Should this be rendered in API docstring? Looks like a note meant for internal devs only. Should this be moved to code comment instead?

Since this sounds like such an important function, please also document Parameters and Returns as laid out by numpydoc.

pllim · 2024-11-01T18:43:40Z

jdaviz/utils.py

+    units appear in the aperture photometry output table in the variance columns.
+    Units like (MJy / sr)**2 to (Jy / sr)**2 can convert directly using astropy
+    units , but if an equivalency is required this conversion needs
+    a workaround.


Same comment about Parameters and Returns.

cshanahan1 · 2024-11-01T19:11:57Z

Does this need a change log?

yes waiting until the end so I don't get conflicts to resolve

gibsongreen · 2024-11-04T16:53:58Z

jdaviz/core/marks.py

+                y = flux_conversion_general(self.y, self.yunit, unit, eqv)
            else:
-                y = (self.y * self.yunit).to_value(unit)
+                y = flux_conversion_general(self.y, self.yunit, unit)


Good catch here, this originally was causing one of the tracebacks with some of the plugins in Specviz2d (in #3253), would only go away with this new flux_conversion_general.

gibsongreen · 2024-11-04T16:54:05Z

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

                    if 'PIXAR_SR' in self.app.data_collection[0].meta:
-                        # Need current slice value and associated unit to use to compute
-                        # spectral density equivalencies that enable Flux to Flux conversions.
-                        # This is needed for units that are not directly convertible/translatable.
-                        slice = viewer.slice_value * u.Unit(self.app._get_display_unit('spectral'))
-
-                        value = flux_conversion(value, unit, self.image_unit,
-                                                eqv=_eqv_pixar_sr(self.app.data_collection[0].meta['PIXAR_SR']),  # noqa: E501
-                                                slice=slice)
-                        unit = self.image_unit
-
-                    elif self.image_unit.is_equivalent(unit):
-                        value = (value * u.Unit(unit)).to_value(u.Unit(self.image_unit))
-                        unit = self.image_unit
+                        pixar_sr = self.app.data_collection[0].meta
+                    else:
+                        pixar_sr = 1
+                    eqv += _eqv_pixar_sr(pixar_sr)


Is there a case here where we might need the _eqv_flux_to_sb_pixel equivalencies? This applies to Specviz, Specviz2d, (and I'm assuming Mosviz). This is one outcome in Specviz without it in the image below. In #3253 I introduce 'un'-exposing the SB and angle unit so they don't display in the API/UI, so it might not need to be address but until that is in this is the current state in non-Cubeviz configs.

gibsongreen · 2024-11-04T16:54:08Z

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

+                valid_types = ["spectral flux density", "surface brightness",
+                               "power density/spectral flux density wav",
+                               "photon flux density wav",
+                               "photon flux density"]
+                if str(physical_type) not in valid_types:


Will make a note on the physical types JDAT-ticket and here (#3234).

gibsongreen · 2024-11-04T16:54:18Z

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

        equivs = _eqv_flux_to_sb_pixel() + u.spectral_density(init_x)
-        init_y = init_y.to(self._units['y'], equivs)
+
+        init_y = flux_conversion_general([init_y.value], init_y.unit, self._units['y'], equivs)


Leaving another note here to retest model fitting in Specviz2d following merging this PR (#3253). Bug was preexisting unit conversion plugin but still worth retesting.

gibsongreen · 2024-11-04T16:54:20Z

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

+        eqv = _eqv_pixar_sr(self.dataset.selected_obj.meta.get('PIXAR_SR', 1.0))
+        eqv += _eqv_flux_to_sb_pixel()  # for conversions between flux <> sb per pix2
+        eqv += u.spectral_density(self.dataset.selected_obj.spectral_axis)


Suggested change

eqv = _eqv_pixar_sr(self.dataset.selected_obj.meta.get('PIXAR_SR', 1.0))

eqv += _eqv_flux_to_sb_pixel() # for conversions between flux <> sb per pix2

eqv += u.spectral_density(self.dataset.selected_obj.spectral_axis)

eqv = (

_eqv_pixar_sr(self.dataset.selected_obj.meta.get('PIXAR_SR', 1.0))

+ _eqv_flux_to_sb_pixel()

+ u.spectral_density(self.dataset.selected_obj.spectral_axis)

)

gibsongreen · 2024-11-04T16:54:28Z

jdaviz/utils.py

+def flux_conversion_general(values, original_unit, target_unit,
+                            equivalencies=None, with_unit=True):


Should we keep the parameters the same here as in flux_conversion? Originally adding spec and slice had to do with the image-viewers (the one case I explicitly remember testing for is working with the current state here) and eventually image data. I'll need to test this out a bit more to, might not be necessary but I will add a note about this on the Imviz implementation ticket.

github-actions bot added cubeviz testing imviz plugin Label for plugins common to multiple configurations labels Oct 18, 2024

gibsongreen reviewed Oct 18, 2024

View reviewed changes

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

moving changes

12c5b13

cshanahan1 force-pushed the new_fix_plugin_equivs branch from dc6fc9c to 12c5b13 Compare November 1, 2024 13:50

cshanahan1 marked this pull request as ready for review November 1, 2024 13:51

cshanahan1 requested review from rosteen, javerbukh, pllim, kecnry, bmorris3 and haticekaratay as code owners November 1, 2024 13:51

code style

5bc90f9

cshanahan1 added this to the 4.1 milestone Nov 1, 2024

pllim reviewed Nov 1, 2024

View reviewed changes

gibsongreen reviewed Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix flux unit conversions in plugins #3228

fix flux unit conversions in plugins #3228

cshanahan1 commented Oct 18, 2024 •

edited

Loading

pllim commented Nov 1, 2024

pllim commented Nov 1, 2024

pllim Nov 1, 2024

pllim Nov 1, 2024

pllim Nov 1, 2024

pllim Nov 1, 2024

cshanahan1 commented Nov 1, 2024

gibsongreen Nov 4, 2024

gibsongreen Nov 4, 2024

gibsongreen Nov 4, 2024

gibsongreen Nov 4, 2024

gibsongreen Nov 4, 2024 •

edited

Loading

gibsongreen Nov 4, 2024

		def flux_conversion_general(values, original_unit, target_unit,
		equivalencies=None, with_unit=True):

fix flux unit conversions in plugins #3228

Are you sure you want to change the base?

fix flux unit conversions in plugins #3228

Conversation

cshanahan1 commented Oct 18, 2024 • edited Loading

pllim commented Nov 1, 2024

pllim commented Nov 1, 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

cshanahan1 commented Nov 1, 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 Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshanahan1 commented Oct 18, 2024 •

edited

Loading

gibsongreen Nov 4, 2024 •

edited

Loading