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-33193: Exposure.getReadoutCorner method returns incorrect values for some LSST CCD amplifiers. #625
Conversation
1311728
to
42f2739
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.
Looks good, overall. Just some minor comments about where the changes should go, assuming that the code will still be correct. Is it fairly straightforward to add a few lines in test_amplifier.py
that checks that the readout corners are coming out correctly?
# Update the Readout Corner if we've flipped anything. | ||
outReadoutCorner = self.getReadoutCorner() | ||
if self.getRawFlipX() != outFlipX: | ||
xFlipMapping = {ReadoutCorner.LL: ReadoutCorner.LR, ReadoutCorner.LR: ReadoutCorner.LL, |
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.
Would it make more sense to do this operation in L166, so it happens right after bbox
is flipped?
ReadoutCorner.UR: ReadoutCorner.UL, ReadoutCorner.UL: ReadoutCorner.UR} | ||
outReadoutCorner = xFlipMapping[outReadoutCorner] | ||
if self.getRawFlipY() != outFlipY: | ||
yFlipMapping = {ReadoutCorner.LL: ReadoutCorner.UL, ReadoutCorner.LR: ReadoutCorner.UR, |
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.
Similarly, this could happen after current L180.
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 wanted to do these two checks outside of the bbox loop. My logic was to do all of the pixel based flips, and then set the amplifier parameters. The only constraint is that the setReadoutCorner
call needs to happen before the setRawFlip*
calls.
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 meant this could go before the loop. The bbox
loop begins only at L171, right?
42f2739
to
951e167
Compare
I agree on the unit test comment, and have added two lines to the existing test that check that the readout corners match expectations. |
Update the amplifier readout corner if data was flipped.