add pixel scale factor to metadata and API translation #2752

gibsongreen · 2024-03-13T02:51:17Z

Description

This pull request is to address storing the scale factor in metadata so that data can be "translated" between surface brightness and flux units. In addition, to add internal API functionality to translate from MJy/sr <-> Jy/pix.

Screen.Recording.2024-03-19.at.1.30.07.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)?

codecov · 2024-03-13T03:07:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.70%. Comparing base (9edd445) to head (bf012bd).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2752      +/-   ##
==========================================
- Coverage   88.70%   88.70%   -0.01%     
==========================================
  Files         108      108              
  Lines       16305    16374      +69     
==========================================
+ Hits        14464    14524      +60     
- Misses       1841     1850       +9

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

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

pllim · 2024-03-13T13:59:52Z

CHANGES.rst

So far, none of the diff is user-facing, so it does not need a change log.

kecnry · 2024-03-19T17:29:03Z

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

@@ -159,7 +160,7 @@ def user_api(self):
        expose = ['function', 'spatial_subset', 'aperture',
                  'add_results', 'collapse_to_spectrum',
                  'wavelength_dependent', 'reference_wavelength',
-                  'aperture_method']
+                  'aperture_method', 'translate_units']


I'd vote to not expose this to the user yet.

The list name makes more sense now, will do and make a note of it.

@kecnry how do I hide it from the user, but still enable it to be callable?

remove it from this list, and either access the plugin object directly (if you already have the user API object, just call ._obj on it to get to the actual plugin object).

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

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

+            collapsed_spec._data *= 10e6
+
+            # spectral_cube.meta['_pixel_scale_factor'] / 1, where 1 = default scale factor
+            collapsed_spec._data *= spectral_cube.meta['_pixel_scale_factor']


I think the scale factors will need to be stored in collapsed_spec instead of spectral_cube, since a single spectral cube can be used to extract multiple spectra with different subsets/apertures.

You are right, that makes a lot more sense instead of the flux data!

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

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

+        elif collapsed_spec._unit != u.Jy / u.pix and collapsed_spec._unit != u.MJy / u.sr:
+            raise ValueError(f"Units '{collapsed_spec._unit}' are not MJy/sr nor Jy/pix.")


can this not be generalized to any flux or surface brightness units?

Should be doable and generalizing here is a better idea

kecnry · 2024-03-19T17:45:15Z

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

+            # per https://jwst-docs.stsci.edu/jwst-near-infrared-camera/nircam-performance/nircam-absolute-flux-calibration-and-zeropoints # noqa
+            # MJy/sr is default, to measure photometry in MJy, convert the image to units
+            # of MJy/pixel by multiplying by the average area of a pixel in sr, a constant
+            # provided in the FITS header keyword PIXAR_SR
+            collapsed_spec._data *= spectral_cube.meta['PIXAR_SR']


should this, and the original unit, be absorbed into the scale factor so we can just multiply or divide by a quantity?

kecnry · 2024-03-20T19:30:52Z

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

@@ -343,6 +345,11 @@ def collapse_to_spectrum(self, add_data=True, **kwargs):
                sender=self)
            self.hub.broadcast(snackbar_message)

+        # per https://jwst-docs.stsci.edu/jwst-near-infrared-camera/nircam-performance/nircam-absolute-flux-calibration-and-zeropoints # noqa
+        if 'PIXAR_SR' in spectral_cube.meta:
+            pix_scale_factor = self.aperture.scale_factor * 10e6 * spectral_cube.meta['PIXAR_SR']


where does the 10^6 come from? Does that make assumptions on units (converting between MJy and Jy maybe)?

I had made the assumption based on conversations from the hack day and the meetings with Patrick that yes MJy to Jy would need scientific notation conversion, I'll remove it from here and the specification in the translate_units() function

it would, but we're going to want to have the flexibility to change between any prefix. Ideally this would take the new units, but if that's too difficult, we can do that as a followup and just assume the same units (except the sr) otherwise.

kecnry · 2024-03-20T19:32:38Z

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

+        # MJy/sr -> Jy/pix
+        if collapsed_spec._unit == u.MJy / u.sr:
+            collapsed_spec._data *= collapsed_spec.meta['_pixel_scale_factor']
+            collapsed_spec._unit = u.Jy / u.pix
+        # Jy/pix -> MJy/sr
+        elif collapsed_spec._unit == u.Jy / u.pix:
+            collapsed_spec._data /= collapsed_spec.meta['_pixel_scale_factor']
+            collapsed_spec._unit = u.MJy / u.sr


I still think we want this to be generalized to any surface brightness -> flux units, basically just adding or removing the steradians but keeping the same base units otherwise (which probably means we don't need the 10^6 in the factor).

gibsongreen · 2024-03-21T15:24:17Z

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

+            u.MJy / u.sr: u.MJy / u.pix,
+            u.Jy / u.sr: u.Jy / u.pix,
+            u.erg / (u.cm**2*u.s*u.AA*u.sr): u.erg / (u.cm**2*u.s*u.AA),
+            u.erg / (u.s*u.cm**2*u.sr): u.erg / (u.s*u.cm**2),
+            u.W / (u.m**2*u.Hz*u.sr): u.W / (u.m**2*u.Hz)


The dictionary will probably have to go when translating to any sb/flux. If there's eventually a drop down for the user to select their desired units, the path forward for this becomes clear.

can we just multiply or divide by u.sr directly?

kecnry · 2024-03-21T15:57:52Z

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

@@ -343,6 +345,11 @@ def collapse_to_spectrum(self, add_data=True, **kwargs):
                sender=self)
            self.hub.broadcast(snackbar_message)

+        # per https://jwst-docs.stsci.edu/jwst-near-infrared-camera/nircam-performance/nircam-absolute-flux-calibration-and-zeropoints # noqa
+        if 'PIXAR_SR' in spectral_cube.meta:


what happens when 'PIXAR_SR' does not exist? What if instead we just use spectral_cube.meta.get('PIXAR_SR', 1.0)?

I updated this instead of the if statement, I think we'd need to calculate the average area of a pixel if PIXAR_SR doesn't exist

kecnry · 2024-03-21T15:58:38Z

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

+            u.MJy / u.sr: u.MJy / u.pix,
+            u.Jy / u.sr: u.Jy / u.pix,
+            u.erg / (u.cm**2*u.s*u.AA*u.sr): u.erg / (u.cm**2*u.s*u.AA),
+            u.erg / (u.s*u.cm**2*u.sr): u.erg / (u.s*u.cm**2),
+            u.W / (u.m**2*u.Hz*u.sr): u.W / (u.m**2*u.Hz)


can we just multiply or divide by u.sr directly?

kecnry · 2024-03-21T18:23:04Z

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

+        # per https://jwst-docs.stsci.edu/jwst-near-infrared-camera/nircam-performance/nircam-absolute-flux-calibration-and-zeropoints # noqa
+            pix_scale_factor = self.aperture.scale_factor * spectral_cube.meta.get('PIXAR_SR', 1.0)
+            collapsed_spec.meta['_pixel_scale_factor'] = pix_scale_factor


I'm torn whether this belong in or outside of the if add_data... but might be useful for comparing if its outside (and then just in case anyone passes that data back into jdaviz)

Suggested change

# per https://jwst-docs.stsci.edu/jwst-near-infrared-camera/nircam-performance/nircam-absolute-flux-calibration-and-zeropoints # noqa

pix_scale_factor = self.aperture.scale_factor * spectral_cube.meta.get('PIXAR_SR', 1.0)

collapsed_spec.meta['_pixel_scale_factor'] = pix_scale_factor

# per https://jwst-docs.stsci.edu/jwst-near-infrared-camera/nircam-performance/nircam-absolute-flux-calibration-and-zeropoints # noqa

pix_scale_factor = self.aperture.scale_factor * spectral_cube.meta.get('PIXAR_SR', 1.0)

collapsed_spec.meta['_pixel_scale_factor'] = pix_scale_factor

oh but this needs to be done before add_results_from_plugin, right now the metadata entry isn't showing up when accessing through viz.get_data(...).

kecnry · 2024-03-21T18:26:14Z

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

@@ -519,3 +524,13 @@ def _live_update(self, event={}):
        for mark in self.marks.values():
            mark.update_xy(sp.spectral_axis.value, sp.flux.value)
            mark.visible = True
+
+    def translate_units(self, collapsed_spec):


this is probably fine for this PR to demonstrate the translation, but when we connect a switch, we'll want to have the ability to change the entry already loaded in the data-collection (preferably in-place without having to re-parse).

kecnry

LGTM - definitely will want science verification on a real workflow but that can be done once the whole thing is functional (in the planned follow-up efforts). Thanks!

gibsongreen · 2024-03-21T19:06:12Z

LGTM - definitely will want science verification on a real workflow but that can be done once the whole thing is functional (in the planned follow-up efforts). Thanks!

I most definitely agree, thank you for all of your productive input!

haticekaratay

Looks good to me! Thanks for your work!

…lity

…r with scale factor

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

gibsongreen added this to the 3.9 milestone Mar 13, 2024

pllim reviewed Mar 13, 2024

View reviewed changes

gibsongreen marked this pull request as ready for review March 19, 2024 17:40

gibsongreen requested review from rosteen, javerbukh, kecnry, bmorris3, haticekaratay and cshanahan1 as code owners March 19, 2024 17:40

kecnry reviewed Mar 19, 2024

View reviewed changes

gibsongreen added the no-changelog-entry-needed changelog bot directive label Mar 19, 2024

kecnry reviewed Mar 20, 2024

View reviewed changes

gibsongreen commented Mar 21, 2024

View reviewed changes

kecnry reviewed Mar 21, 2024

View reviewed changes

kecnry approved these changes Mar 21, 2024

View reviewed changes

kecnry added the Ready for final review label Mar 21, 2024

haticekaratay approved these changes Mar 22, 2024

View reviewed changes

gibsongreen added 6 commits March 22, 2024 13:43

add pixel scale factor to metadata

da5352f

add change log

f9d8e75

remove change log and update to use flux cube metadata

b1a2eca

add translation functionality to API, and test two-way test functiona…

e8861c3

…lity

move scale factor to collapsed_spec, combine sci notation and pixar_s…

a2fb178

…r with scale factor

unexpose translate_units() from user

299308e

gibsongreen added 3 commits March 22, 2024 13:43

remove hard-coded sci notation, add additional flux/sb units

b01aa90

removing unit dictionary, multiply/divide by sr

270123a

moving metadata addition to before add_results_from_plugin

bf012bd

gibsongreen force-pushed the store_pixel_scale branch from f10c122 to bf012bd Compare March 22, 2024 17:44

gibsongreen added the skip-changelog-checks changelog bot directive label Mar 22, 2024

kecnry merged commit d007a4b into spacetelescope:main Mar 25, 2024
18 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add pixel scale factor to metadata and API translation #2752

add pixel scale factor to metadata and API translation #2752

gibsongreen commented Mar 13, 2024 •

edited

Loading

codecov bot commented Mar 13, 2024 •

edited

Loading

pllim Mar 13, 2024

kecnry Mar 19, 2024

gibsongreen Mar 19, 2024

gibsongreen Mar 19, 2024 •

edited

Loading

kecnry Mar 19, 2024

kecnry Mar 19, 2024

gibsongreen Mar 19, 2024

kecnry Mar 19, 2024

gibsongreen Mar 19, 2024

kecnry Mar 19, 2024

kecnry Mar 20, 2024

gibsongreen Mar 20, 2024

kecnry Mar 20, 2024

kecnry Mar 20, 2024

gibsongreen Mar 21, 2024

kecnry Mar 21, 2024

kecnry Mar 21, 2024

gibsongreen Mar 21, 2024

kecnry Mar 21, 2024

kecnry Mar 21, 2024

kecnry Mar 21, 2024

kecnry Mar 21, 2024

kecnry left a comment

gibsongreen commented Mar 21, 2024

haticekaratay left a comment

		elif collapsed_spec._unit != u.Jy / u.pix and collapsed_spec._unit != u.MJy / u.sr:
		raise ValueError(f"Units '{collapsed_spec._unit}' are not MJy/sr nor Jy/pix.")

add pixel scale factor to metadata and API translation #2752

add pixel scale factor to metadata and API translation #2752

Conversation

gibsongreen commented Mar 13, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Mar 13, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibsongreen Mar 19, 2024 • edited Loading

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

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

kecnry left a comment

Choose a reason for hiding this comment

gibsongreen commented Mar 21, 2024

haticekaratay left a comment

Choose a reason for hiding this comment

gibsongreen commented Mar 13, 2024 •

edited

Loading

codecov bot commented Mar 13, 2024 •

edited

Loading

gibsongreen Mar 19, 2024 •

edited

Loading