Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-8609

Fix misuse of Image.Factory




      It turns out that some packages are calling Image.Factory(image, bbox, bool) where the bool is intended to indicate deepcopy? and Image is any image-like class. This is an error because the overload being called is Image.Factory(image, bbox, origin=PARENT, deepcopy=False) so the boolean is treated as image origin.

      I found this during pybind11 conversion, because pybind11 does not allow ints to be passed as enums.

      This is harmless if the value of bool is False; that False can simply be removed without changing the behavior of the code. However, if the value of bool is True then the code is probably actually doing the wrong thing – returning a subregion that is LOCAL and a shallow copy, instead of PARENT and a deep copy. Such code needs to be carefully studied, and this ticket is intended for such incidents.

      Note that the similar constructor Image.Factory(image, deepcopy) is not a concern.

      Using a grep search I found the following examples. more may well be present (for instance I only looked for Factory, rather than calling an image-like constructor directly, and I scanned by eye so I may have missed some. All will show up during the pybind11 conversion, but it would be nice to fix these sooner.

      meas/deblender/baseline.py has several instances, including:

       t1 = t1.Factory(t1, tfoot.getBBox(), afwImage.PARENT, True)
      pkres.psfFitDebugStamp = img.Factory(img, stampbb, True)
      pkres.psfFitDebugVar = varimg.Factory(varimg, stampbb, True)

      ip/diffim/utils.py has some:

      stamp = exposure.Factory(exposure, bbox, True)
      resid = resid.Factory(resid, bbox, True)
      resid = resid.Factory(resid, bbox, True)

      Note that I already fixed all incidences that I found in afw in DM-7801, but these all had bool=False so the fix was trivial: simply remove the False to preserve the same behavior with a more correct call signature.


          Issue Links


            rowen Russell Owen added a comment -

            This is being fixed as part of DM-8467

            rowen Russell Owen added a comment - This is being fixed as part of DM-8467


              rowen Russell Owen
              rowen Russell Owen
              Kian-Tat Lim, Pim Schellart [X] (Inactive), Russell Owen
              0 Vote for this issue
              3 Start watching this issue