New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RAD-158: Changed image units. #389
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
=======================================
Coverage 95.38% 95.38%
=======================================
Files 4 4
Lines 195 195
=======================================
Hits 186 186
Misses 9 9 ☔ View full report in Codecov by Sentry. |
CHANGES.rst
Outdated
@@ -16,6 +16,8 @@ | |||
|
|||
- Updated product table names. [#382] | |||
|
|||
- Changed image units. [#389] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be good for the release notes to specify what the change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -26,9 +26,10 @@ properties: | |||
tag: asdf://stsci.edu/datamodels/roman/tags/source_detection-1.0.0 | |||
required: [photometry, wcs] | |||
data: | |||
title: Science Data (electrons / s) | |||
title: Science Data (DN / s) or (MJy / sr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the conversion to MJy/sr applied in Level 2 or L3? If L2 (and I may be confused about this) then remove MJy/sr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L2, and the MJy/sr option was specifically requested in the ticket.
oneOf: | ||
- tag: tag:stsci.edu:asdf/unit/unit-1.* | ||
- tag: tag:astropy.org:astropy/units/unit-1.* | ||
enum: ["DN / s", "MJy.sr**-1"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@braingram Can one of the tags be dropped here? I believe DN/s is a VO unit and supported by astropy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping!
The oneOf
here is needed as asdf-astropy
will select which tag to use based on the unit (and if it is or is not a VO unit). See the relevant converter code here.
Writing out a ImageModel
with DN / s
data produces the following and uses the astropy tag:
data: !unit/quantity-1.1.0
unit: !<tag:astropy.org:astropy/units/unit-1.0.0> DN / s
value: !core/ndarray-1.0.0
source: 9
datatype: float32
byteorder: little
shape: [4088, 4088]
whereas using MJy.sr**-1
produces the following and uses the stsci tag:
data: !unit/quantity-1.1.0
unit: !unit/unit-1.0.0 MJy.sr**-1
value: !core/ndarray-1.0.0
source: 9
datatype: float32
byteorder: little
shape: [4088, 4088]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh! My bad! I checked DN/s but not MJy/sr as I thought that surely this is a VO unit. Apparently not!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Resolves RAD-158
Closes #388
This PR changes the units of the variables within the Image Model schema.
NOTE: no regression test link, because these changes require update to regression test files.
Checklist
CHANGES.rst
under the corresponding subsection