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-37205: Confirm quantities needed for image quality analysis from ISR are in postIsrCcd #246
Conversation
for amp in ccd: | ||
ampExposure = ccdExposure.Factory(ccdExposure, amp.getBBox()) | ||
ampName = amp.getName() | ||
metadata[f"LSST ISR MASK SAT {ampName}"] = isrFunctions.countMaskedPixels( |
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 prefixing these by LSST
needed/desirable?
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.
Namespacing FITS HIERARCH headers with a observatory prefix is a convention to guarantee that you can't clash with anyone else. I don't see much of a downside in the "LSST" usage.
python/lsst/ip/isr/isrTask.py
Outdated
|
||
metadata[f"{keyBase} PAR MEAN {ampName}"] = overscanResults.overscanMean[1] | ||
metadata[f"{keyBase} PAR MEDIAN {ampName}"] = overscanResults.overscanMedian[1] | ||
metadata[f"{keyBase} PAR STDEV {ampName}"] = overscanResults.overscanSigma[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.
I'm asking from a position of ignorance here. Is PAR
common for parallel, or should we have it fully as PARALLEL
for clarity?
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.
PARALLEL
is definitely clearer. I'll make that switch.
k2 = f"LSST ISR OVERSCAN SERIAL MEDIAN {ampName}" | ||
metadata[f"LSST ISR LEVEL {ampName}"] = metadata[k1] - metadata[k2] | ||
else: | ||
metadata[f"LSST ISR LEVEL {ampName}"] = numpy.nan |
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.
What happens to metadata
at the end of the loop? Shouldn't it be linked to self.metadata
before the next iteration? Or are these available the next time ccdExposure.getMetadata()
is called?
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.
They should be immediately available. getMetadata
gives a pointer (for lack of a better term on my part) to the exposure metadata, and repeated calls yield identical pointers:
exp = afwImage.ExposureF.readFits(filename)
mm1 = exp.getMetadata()
mm2 = exp.getMetadata()
print(repr(mm1), repr(mm2))
## <lsst.daf.base.propertyContainer.propertyList.PropertyList object at 0x7f60dc04f030> <lsst.daf.base.propertyContainer.propertyList.PropertyList object at 0x7f60dc04f030>
I've previously put information into self.metadata
(the task's metadata), but I think that's always been a bad decision, and this is the start of fixing that.
python/lsst/ip/isr/isrTask.py
Outdated
metadata[f"{keyBase} RESIDUAL PAR MEDIAN {ampName}"] = overscanResults.residualMedian[1] | ||
metadata[f"{keyBase} RESIDUAL PAR STDEV {ampName}"] = overscanResults.residualSigma[1] | ||
else: | ||
self.log.warning("Unknown length of overscan values; none added to header.") |
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.
The warning message seems misleading. This warning will be emitted if overscanResults.overscanMean
is neither a float
nor a tuple
but this message says it's of unknown length. It should be something like "Overscan values are of unacceptable type; none added to header.")
Add image quality values to image header.