Address review comments

python/lsst/ip/isr/ampOffset.py

@@ -28,7 +28,7 @@
 from lsst.afw.table import SourceTable
 from lsst.meas.algorithms import SourceDetectionTask, SubtractBackgroundTask
 from lsst.pex.config import Config, ConfigurableField, Field
-from lsst.pipe.base import Task, Struct
+from lsst.pipe.base import Struct, Task
 
 
 class AmpOffsetConfig(Config):
@@ -157,19 +157,23 @@ def run(self, exposure):
             self.interfaceLengthLookupBySide = {i: self.ampDims[i % 2] for i in range(4)}
 
         # Determine amplifier geometry.
-        # ampWidths = {amp.getBBox().getWidth() for amp in amps}
-        # ampHeights = {amp.getBBox().getHeight() for amp in amps}
+        ampWidths = {amp.getBBox().getWidth() for amp in amps}
+        ampHeights = {amp.getBBox().getHeight() for amp in amps}
         ampAreas = {amp.getBBox().getArea() for amp in amps}
-        if len(ampAreas) > 1:
+        if len(ampWidths) > 1 or len(ampHeights) > 1 or len(ampAreas) > 1:
             raise NotImplementedError(
                 "Amp offset correction is not yet implemented for detectors with differing amp sizes."
             )
 
         # Assuming all the amps have the same geometry.
         self.shortAmpSide = np.min(ampDims[0])
 
-        # TODO: Double check that this is true.
-        assert self.config.ampEdgeWidth < self.shortAmpSide - 2 * self.config.ampEdgeInset
+        # Check that the edge width and inset are not too large.
+        if self.config.ampEdgeWidth >= self.shortAmpSide - 2 * self.config.ampEdgeInset:
+            raise RuntimeError(
+                f"The edge width ({self.config.ampEdgeWidth}) plus insets ({self.config.ampEdgeInset}) "
+                f"exceed the amp's short side ({self.shortAmpSide}). This setup leads to incorect results."
+            )
 
         # Fit and subtract background.
         if self.config.doBackground:
@@ -202,9 +206,6 @@ def run(self, exposure):
                 "All pixels masked: cannot calculate any amp offset corrections. All pedestals are being set "
                 "to zero."
             )
-            print(
-                "All pixels masked: cannot calculate any amp offset corrections. All pedestals are being set"
-            )
             pedestals = np.zeros(len(amps))
         else:
             # Set up amp offset inputs.
@@ -215,7 +216,7 @@ def run(self, exposure):
                 raise RuntimeError(
                     f"The specified fraction (`ampEdgeWindowFrac`={self.config.ampEdgeWindowFrac}) of the "
                     "edge length exceeds 1. This leads to complications downstream, after convolution in "
-                    "the `getSideAmpOffset()` method. Please modify the `ampEdgeWindowFrac` value in the "
+                    "the `getInterfaceOffset()` method. Please modify the `ampEdgeWindowFrac` value in the "
                     "config to be 1 or less and rerun."
                 )
 
@@ -483,12 +484,18 @@ def getInterfaceOffset(self, ampIdA, ampIdB, edgeA, edgeB):
         maxOffsetFail = np.abs(interfaceOffset) > self.config.ampEdgeMaxOffset
         if minFracFail or maxOffsetFail:
             interfaceOffset = 0
-            self.log.warning(
-                f"The fraction of unmasked pixels for amp interface {interfaceId} is below the threshold "
-                f"({ampEdgeGoodFrac:.2f} <? {self.config.ampEdgeMinFrac}) or the absolute offset value "
-                f"exceeds the limit ({np.abs(interfaceOffset):.2f} >? {self.config.ampEdgeMaxOffset} ADU). "
-                f"Setting the interface offset to {interfaceOffset}."
-            )
+            if minFracFail:
+                self.log.warning(
+                    f"The fraction of unmasked pixels for amp interface {interfaceId} is below the threshold "
+                    f"({ampEdgeGoodFrac:.2f} < {self.config.ampEdgeMinFrac}). Setting the interface offset "
+                    f"to {interfaceOffset}."
+                )
+            if maxOffsetFail:
+                self.log.warning(
+                    "The absolute offset value exceeds the limit "
+                    f"({np.abs(interfaceOffset):.2f} > {self.config.ampEdgeMaxOffset} ADU). Setting the "
+                    f"interface offset to {interfaceOffset}."
+                )
         self.log.debug(
             f"amp interface {interfaceId} : "
             f"viable edge difference frac = {ampEdgeGoodFrac}, "

tests/test_ampOffset.py

@@ -237,11 +237,10 @@ def buildExposure(self, valueType, addBackground=False, rampBackground=False):
             will not solve exactly.
 
         addBackground : `bool`, optional
-            If True, adds a background value to the entire exposure. Default is
-            False.
+            If True, adds a background value to the entire exposure.
 
         rampBackground : `bool`, optional
-            Whether the added background should be a ramp. Default is False.
+            Whether the added background should be a ramp.
 
         Returns
         -------
@@ -287,7 +286,7 @@ def runAmpOffsetWithBackground(self, valueType, rampBackground=False):
             See `buildExposure` for details.
 
         rampBackground : `bool`
-            Whether the added background should be a ramp. Default is False.
+            Whether the added background should be a ramp.
         """
         if rampBackground:
             measuredPedestals = self.measuredPedestalsRampBackground