DM-31760: Single amp reads for obs_lsst have the wrong geometry. #345
Conversation
python/lsst/obs/lsst/rawFormatter.py
Outdated
| @@ -178,7 +178,7 @@ def findAmpHdu(name): | |||
| # on-disk amplifier to have the same orientation and offsets as | |||
| # the given one. | |||
| adjusted_amplifier_builder.transform( | |||
| outOffset=amplifier.getRawXYOffset(), | |||
| outOffset=adjusted_amplifier_builder.getRawXYOffset(), | |||
There was a problem hiding this comment.
Could we equivalently write this as on_disk_amplifier.getRawXYOffset()? I think that'd be a tiny bit clearer.
Separately, I'm really kind of surprised that it's okay to just this offset here - I can see why the previous code wouldn't have worked when the overscans need to be adjusted, but I would not have thought one could use the (e.g.) on-disk, readout-coordinates offset to set up an amplifier that the user might be requesting in physical coordinates. Is getRawXYOffset not something that changes in assembly when we go from readout coordinates to physical coordinates?
There was a problem hiding this comment.
Agree on the clarification.
I don't believe RawXYOffset changes anywhere other than in the fixAmpGeometry function. The issue is that the transform method defaults to moving all the boxes to their assembled location, and so updates all the Raw boxes by bbox.shift(self.getRawXYOffset() + shift - outOffset). For the single-amp read case, we need to cancel that move.
There was a problem hiding this comment.
Ok; I'm not sure I completely follow, but I'm happy to go with whatever makes the most sense to you in terms of overall behavior.
| comparison2 = unassembled_subimage.getDetector()[0].compareGeometry(unassembled_amp) | ||
| self.assertFalse(comparison2 & AmplifierGeometryComparison.ASSEMBLY_DIFFERS) |
There was a problem hiding this comment.
If this test now fails, I think it means that I defined ASSEMBLY_DIFFERS incorrectly, i.e. it includes parameters that are really part of the fundamental region geometry rather than part of the assembly state. If that's the case, we really should change it, too, but then there might be a lot more logic (especially upstream in obs_base) that needs to be fixed.
There was a problem hiding this comment.
The specific case it fails is SHIFTED_X. That still makes sense to me as indicating something is different about the assembly, because this comparison is between two things that are different in their assembly: the amplifier as part of a full detector vs the amplifier alone.
If the detector has been modified, then any transforms need to be relative to the new XY offset position that is calculated during that modification.
After modifying the amplifier, there may be shifts between it and the camera-derived amplifier. As this isn't simply changing a assertTrue to assertFalse (as the first amp in the axis that is modified will actually have shift=0), removing the test seems the only solution.
czwa
force-pushed
the
tickets/DM-31760
branch
from
b386cb6
to
5b7e6e3
Compare
Fix transform for single-amp reads.
If the detector has been modified, then any transforms need to be
relative to the new XY offset position that is calculated during that
modification.