-
Notifications
You must be signed in to change notification settings - Fork 6
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
DM-29370: support for new detector component and 'amp' parameter in raw formatters #313
Conversation
c024854
to
f004301
Compare
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.
I'm slightly concerned that we only warn if we read a extension with one when we expect another, but I don't know how much that warning triggers (I can imagine it only happens for very old data).
extname = metadata.get("EXTNAME") | ||
predictedExtname = f"Segment{inAmp.getName()[1:]}" | ||
if extname is not None and predictedExtname != extname: | ||
logger.warning('expected to see EXTNAME == "%s", but saw "%s"', predictedExtname, extname) |
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.
I know this is just a migration of this check, but if I do butler.get('raw', {'amp': 'R12_S00'})
, isn't it an error if I receive something else? I can see in the full-detector case that getting the wrong thing might be fine, because each amp will mosaic into the right place eventually, but I'm not sure that's the case for single-amp reads.
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.
amp
doesn't take a detector name does it? I haven't looked at the code but shouldn't the amp parameter be an integer?
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.
From the obs_base
ticket branch, formatters/fitsExposure.py
line 458 seems to allow an integer or a string,
And now I see my typo, using a detector name instead of an amp name. That should be something like C10
.
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.
I assume this is a guard against incorrect headers or something, but I don't actually know; the check/warning was there before, and all I did here was move it around and make it actually work.
This makes these checks more reusable in other contexts.
Support for raw.detector is new in obs_base, but requires work for LSST because we modify the detectors when raw images are read. But it's also more important for LSST, because this gives us a way to get a detector with the right geometry for raw, without actually reading the image.
Downstream functions like fixAmpsAndAssemble were expecting per-amp header metadata to be passed in, but were getting empty PropertyLists because we read the data from disk via Image, not Exposure. We never noticed because they were designed to do nothing when those header keys were missing. This did not affect the metadata ultimately associated with raws (at least in Gen3) because we read that separately.
f004301
to
ee7c68d
Compare
No description provided.