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

Investigate proper precision for afw::image::Image pixel transforms

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      2
    • Team:
      Data Release Production
    • Urgent?:
      No

      Description

      The various pixel based transforms in afw/src/image/Image.cc were converted from using boost::lambda to C++11 lambda per DM-6091.

      At many places the previous implementation contained implicit casts (through boost::ret) of intermediate results to PixelT (e.g. float).
      In particular this affects opperations such as result = l + c*r where l is the left hand side image, r is the right hand side image and c a double constant.
      When calculated at double precision (e.g. without the casts, which are not needed with C++11 lambdas) the result is slightly different and this causes tests/testProcessCcd.py to fail on self.assertAlmostEqual(psfIyy, 2.17386182921239, places=7) which is only equal up to the fifth place.

      In order to not break existing behaviour I added explicit casts to PixelT for intermediate results. But this approach is questionable as the end result will be less accurate then possible. The aim of this ticket is to decide which approach is best:

      1. Calculate at full precision and modify the test case.
      2. Cast intermediate results to final precision (as it is done now).
      3. Do something else?

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment - - edited

            Does this look like something that could be executed inside a tight loop? If so, switching from float to double when increased precision is not needed could represent a performance regression.

            Show
            jbosch Jim Bosch added a comment - - edited Does this look like something that could be executed inside a tight loop? If so, switching from float to double when increased precision is not needed could represent a performance regression.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Oh yes, performance is definitely an issue here since basically it involves functions like image::Image<PixelT>::scaledPlus which as far as I can tell are the main things we use to compute on images. But I wonder if it then isn't better to have c (the constant) be of PixelT type as well. Anyway if we do want to stick with single precision here I'm not sure if comparing the test results up to 7 places makes sense.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Oh yes, performance is definitely an issue here since basically it involves functions like image::Image<PixelT>::scaledPlus which as far as I can tell are the main things we use to compute on images. But I wonder if it then isn't better to have c (the constant) be of PixelT type as well. Anyway if we do want to stick with single precision here I'm not sure if comparing the test results up to 7 places makes sense.
            Hide
            jbosch Jim Bosch added a comment -

            Yeah, I agree that something needs to change here. It sounds like switching c to PixelT is probably the cleanest solution, removing the illusion that anything is happening in double precision. But we'll want to consider that a bit carefully in case something is relying on some initial calculation happening in double precision (which I doubt).

            Show
            jbosch Jim Bosch added a comment - Yeah, I agree that something needs to change here. It sounds like switching c to PixelT is probably the cleanest solution, removing the illusion that anything is happening in double precision. But we'll want to consider that a bit carefully in case something is relying on some initial calculation happening in double precision (which I doubt).
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            We just talked about it and Jim Bosch, Yusra AlSayyad and I still think we should do the careful consideration.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - We just talked about it and Jim Bosch , Yusra AlSayyad and I still think we should do the careful consideration.
            Hide
            jbosch Jim Bosch added a comment -

            Matthias Wittgen, I'm assigning this ticket to you because it seems like it might be closely related to your new C+17 ticket, and it might provide some context for that or be something you have to deal with for C+17. If not, we'll consider it a lower-priority ticket for some day in the future.

            Show
            jbosch Jim Bosch added a comment - Matthias Wittgen , I'm assigning this ticket to you because it seems like it might be closely related to your new C+ 17 ticket, and it might provide some context for that or be something you have to deal with for C +17. If not, we'll consider it a lower-priority ticket for some day in the future.
            Hide
            wittgen Matthias Wittgen added a comment -

            Thanks Jim Bosch

            Show
            wittgen Matthias Wittgen added a comment - Thanks Jim Bosch
            Hide
            wittgen Matthias Wittgen added a comment -

            Pushed some code using only PixelT and removing all static_cast. This class has some more problems as clang-tidy shows like an endless loop in ctor. I guess the bool deep copy flag is never used.

            Show
            wittgen Matthias Wittgen added a comment - Pushed some code using only PixelT and removing all static_cast. This class has some more problems as clang-tidy shows like an endless loop in ctor. I guess the bool deep copy flag is never used.
            Hide
            wittgen Matthias Wittgen added a comment -

            The changes in DM-6278 pass the pipelines. I not sure what other tests are needed to qualify these changes.

            Show
            wittgen Matthias Wittgen added a comment - The changes in DM-6278 pass the pipelines. I not sure what other tests are needed to qualify these changes.

              People

              Assignee:
              wittgen Matthias Wittgen
              Reporter:
              pschella Pim Schellart [X] (Inactive)
              Watchers:
              Jim Bosch, Matthias Wittgen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins Builds

                  No builds found.