RAD-92: Update visit, l1, and l2 attribute information #481
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #481 +/- ##
=======================================
Coverage 95.81% 95.81%
=======================================
Files 4 4
Lines 215 215
=======================================
Hits 206 206
Misses 9 9 ☔ View full report in Codecov by Sentry. |
PaulHuwe
requested review from
kdupriestsci,
jbrookens,
WilliamJamieson and
a team
as code owners
| tag: tag:stsci.edu:asdf/unit/quantity-1.* | ||
| properties: | ||
| value: | ||
| tag: tag:stsci.edu:asdf/core/ndarray-1.* | ||
| datatype: float32 | ||
| datatype: uint16 |
There was a problem hiding this comment.
@tddesjardins , I hadn't appreciated we were changing the type of these. Presently the 4k by 4k pixels get converted to floats and treated ~the same up until ramp fitting, when these are separated from the science data. That means treating them like floats for the purposes of this image.
That seems like the right behavior to me, and I read this change as saying we shouldn't do that and should instead ignore these pixels for dark subtraction, linearity correction, etc.. That would require more work in the pipeline and I wouldn't do it here.
@PaulHuwe , I'm writing this comment in the context of border_ref_pix_left, but it applies to the other border_pix as well.
It's also true that we treat the amp33 and other pixels differently. The amp33 pixels do not get the same dark subtraction, etc., that the other pixels do, since they're not actual pixels and it's not clear we could do anything meaningful here. I think that's the right treatment as well, but it is awkward that the different reference pixels have different data types.
There was a problem hiding this comment.
I thought we were holding on to a version of the reference pixels that are uncalibrated, i.e., they have not been dark subtracted, linearity corrected, etc.? The IRRC is before any other calibration step, and the intent was that these arrays are copies of the untouched reference pixels (this was a request from specific science teams via the calibration working group). So I think these are supposed to be uint16 pixels in DN. You're right we carry them around a little longer and chop them off of the image after the classical linearity correction, but these are copies of the originals not the ones we chopped off.
There was a problem hiding this comment.
Ah! Correct, indeed, thanks. Okay, this looks fine, sorry! I guess I register that I think carrying these on uncalibrated is somewhat silly but that's water under the bridge.
There was a problem hiding this comment.
That sounds reasonable. Looking at that code raises a few questions in my mind
https://github.com/spacetelescope/jwst_backgrounds/blob/master/jwst_backgrounds/jbt.py#L180
but we should defer that conversation to later.
|
Closing as all changes already merged. |
Resolves RAD-92
Closes #184
This PR updates visit, l1, and l2 attribute information
Tasks
radtests.docs/page.no-changelog-entry-needed.)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types).romancalregression test (https://github.com/spacetelescope/RegressionTests/actions/workflows/romancal.yml) with this branch installed ("git+https://github.com/<fork>/rad@<branch>").roman_datamodelsutilities and tests.News fragment change types:
changes/<PR#>.feature.rst: new featurechanges/<PR#>.bugfix.rst: fixes an issuechanges/<PR#>.doc.rst: documentation changechanges/<PR#>.removal.rst: deprecation or removal of public APIchanges/<PR#>.misc.rst: infrastructure or miscellaneous change