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

Audit and debug current zogy image differencing code

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Story Points:
      10
    • Sprint:
      AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April), AP F21-1 (June), AP F21-2 (July), AP F21-3 (August), AP F21-4 (September)
    • Team:
      Alert Production
    • Urgent?:
      No

      Description

      The current zogy implementation is assumed to have issues in:

      • Understand how the variance plane should propagate in zogy (convolution then whitening decorrelation).
      • Correct padding and centering of images and PSFs in Fourier space opeartions. (numerical accuracy in DFT vs. origin location and periodicity assumptions)
      • Correct calculation of the difference image variance plane. (variance of image k-space components vs. k-space components of variance plane)
      • zogy.py:384 padSize argument usage

        Attachments

          Issue Links

            Activity

            No builds found.
            gkovacs Gabor Kovacs [X] (Inactive) created issue -
            gkovacs Gabor Kovacs [X] (Inactive) made changes -
            Field Original Value New Value
            Link This issue relates to DM-21868 [ DM-21868 ]
            gkovacs Gabor Kovacs [X] (Inactive) made changes -
            Link This issue relates to DM-21920 [ DM-21920 ]
            gkovacs Gabor Kovacs [X] (Inactive) made changes -
            Issue Type Story [ 10001 ] Bug [ 1 ]
            gkovacs Gabor Kovacs [X] (Inactive) made changes -
            Description The current zogy implementation is assumed to have issues in:
             * Understand how the variance plane should propagate in zogy (convolution then whitening decorrelation).
             * Correct padding and centering of images and PSFs in Fourier space opeartions. (numerical accuracy in DFT vs. origin location and periodicity assumptions)

             * Correct calculation of the difference image variance plane. (variance of image k-space components vs. k-space components of variance plane)
            The current zogy implementation is assumed to have issues in:
             * Understand how the variance plane should propagate in zogy (convolution then whitening decorrelation).
             * Correct padding and centering of images and PSFs in Fourier space opeartions. (numerical accuracy in DFT vs. origin location and periodicity assumptions)

             * Correct calculation of the difference image variance plane. (variance of image k-space components vs. k-space components of variance plane)
             * zogy.py:384 padSize argument usage
            gkovacs Gabor Kovacs [X] (Inactive) made changes -
            Description The current zogy implementation is assumed to have issues in:
             * Understand how the variance plane should propagate in zogy (convolution then whitening decorrelation).
             * Correct padding and centering of images and PSFs in Fourier space opeartions. (numerical accuracy in DFT vs. origin location and periodicity assumptions)

             * Correct calculation of the difference image variance plane. (variance of image k-space components vs. k-space components of variance plane)
             * zogy.py:384 padSize argument usage
            The current zogy implementation is assumed to have issues in:
             * Understand how the variance plane should propagate in zogy (convolution then whitening decorrelation).
             * Correct padding and centering of images and PSFs in Fourier space opeartions. (numerical accuracy in DFT vs. origin location and periodicity assumptions)

             * Correct calculation of the difference image variance plane. (variance of image k-space components vs. k-space components of variance plane)
             * {{zogy.py:384}} padSize argument usage
            swinbank John Swinbank made changes -
            Epic Link DM-22635 [ 427744 ]
            swinbank John Swinbank made changes -
            Team Alert Production [ 10300 ]
            swinbank John Swinbank made changes -
            Sprint AP S20-5 (April) [ 986 ]
            Story Points 10
            swinbank John Swinbank made changes -
            Link This issue relates to DM-12917 [ DM-12917 ]
            swinbank John Swinbank made changes -
            Assignee Gabor Kovacs [ gkovacs ]
            swinbank John Swinbank made changes -
            Epic Link DM-22635 [ 427744 ] DM-24342 [ 433029 ]
            gkovacs Gabor Kovacs [X] (Inactive) made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            swinbank John Swinbank made changes -
            Sprint AP S20-5 (April) [ 986 ] AP S20-5 (April), AP S20-6 (May) [ 986, 987 ]
            gkovacs Gabor Kovacs [X] (Inactive) made changes -
            Link This issue is parent task of DM-25115 [ DM-25115 ]
            gkovacs Gabor Kovacs [X] (Inactive) made changes -
            Link This issue is parent task of DM-25174 [ DM-25174 ]
            swinbank John Swinbank made changes -
            Epic Link DM-24342 [ 433029 ] DM-25144 [ 435262 ]
            swinbank John Swinbank made changes -
            Sprint AP S20-5 (April), AP S20-6 (May) [ 986, 987 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June) [ 986, 987, 1019 ]
            swinbank John Swinbank made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June) [ 986, 987, 1019 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July) [ 986, 987, 1019, 1025 ]
            swinbank John Swinbank made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July) [ 986, 987, 1019, 1025 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-3 (August) [ 986, 987, 1019, 1025, 1033 ]
            swinbank John Swinbank made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-3 (August) [ 986, 987, 1019, 1025, 1033 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-4 (September) [ 986, 987, 1019, 1025, 1039 ]
            swinbank John Swinbank made changes -
            Rank Ranked lower
            swinbank John Swinbank made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-4 (September) [ 986, 987, 1019, 1025, 1039 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October) [ 986, 987, 1019, 1025, 1046 ]
            swinbank John Swinbank made changes -
            Epic Link DM-25144 [ 435262 ] DM-26804 [ 439756 ]
            gkovacs Gabor Kovacs [X] (Inactive) made changes -
            Link This issue is duplicated by DM-10513 [ DM-10513 ]
            gkovacs Gabor Kovacs [X] (Inactive) made changes -
            Link This issue is duplicated by DM-11656 [ DM-11656 ]
            gkovacs Gabor Kovacs [X] (Inactive) made changes -
            Link This issue is duplicated by DM-9441 [ DM-9441 ]
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October) [ 986, 987, 1019, 1025, 1046 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November) [ 986, 987, 1019, 1025, 1046, 1052 ]
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November) [ 986, 987, 1019, 1025, 1046, 1052 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-1 (December) [ 986, 987, 1019, 1025, 1046, 1052, 1059 ]
            sullivan Ian Sullivan made changes -
            Rank Ranked higher
            sullivan Ian Sullivan made changes -
            Epic Link DM-26804 [ 439756 ] DM-27910 [ 442603 ]
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-1 (December) [ 986, 987, 1019, 1025, 1046, 1052, 1059 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-2 (January) [ 986, 987, 1019, 1025, 1046, 1052, 1064 ]
            sullivan Ian Sullivan made changes -
            Rank Ranked lower
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-2 (January) [ 986, 987, 1019, 1025, 1046, 1052, 1064 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February) [ 986, 987, 1019, 1025, 1046, 1052, 1072 ]
            sullivan Ian Sullivan made changes -
            Rank Ranked higher
            sullivan Ian Sullivan made changes -
            Reviewers Ian Sullivan [ sullivan ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            sullivan Ian Sullivan added a comment -

            Could you please add a comment describing the work this ticket covered, and in particular any answers to the issues raised in the description.

            Show
            sullivan Ian Sullivan added a comment - Could you please add a comment describing the work this ticket covered, and in particular any answers to the issues raised in the description.
            sullivan Ian Sullivan made changes -
            Epic Link DM-27910 [ 442603 ] DM-29210 [ 459212 ]
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February) [ 986, 987, 1019, 1025, 1046, 1052, 1072 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079 ]
            Hide
            sullivan Ian Sullivan added a comment -

            Per my previous comment, please add the requested comment describing what was accomplished under this ticket, and what the answers were to the issues raised in the description.

            Show
            sullivan Ian Sullivan added a comment - Per my previous comment, please add the requested comment describing what was accomplished under this ticket, and what the answers were to the issues raised in the description.
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084 ]
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-6 (May) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1088 ]
            sullivan Ian Sullivan made changes -
            Rank Ranked lower
            sullivan Ian Sullivan made changes -
            Rank Ranked higher
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-6 (May) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1088 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084 ]
            sullivan Ian Sullivan made changes -
            Rank Ranked lower
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April), AP S21-6 (May) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084, 1088 ]
            sullivan Ian Sullivan made changes -
            Epic Link DM-29210 [ 459212 ] DM-30435 [ 504823 ]
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April), AP S21-6 (May) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084, 1088 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April), AP F21-1 (June) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084, 1096 ]
            sullivan Ian Sullivan made changes -
            Rank Ranked lower
            Hide
            gkovacs Gabor Kovacs [X] (Inactive) added a comment -

            The new implementation refers to the major zogy code rewrite in DM-25115 and its updates since then.

            The old implementation had a bug in how to center the PSF properly in an FFT symmetry sense (DM-11990). The centering of the image before and after FFT transformations got unnecessarily much attention while it had no real importance. In the new implementation, we always put the image center to the DFT origin for better symmetry.

             

            The image padding size got into focus at the beginning for the wrong reason. We displayed the zogy matching kernel (and decorrelation afterburner) kernels in image space and had the impression that they contain long tails stretching up to hundreds of pixels. Later it became clear that this was due to noise in high frequency components which fills the image space representations irrespectively of their padding size. Nevertheless, to avoid the introduction of artifacts due to the circular boundary condition of DFT space calculations, we need to estimate the matching kernel dimensions in image space. See the theoretical Gaussian solutions in DMTN-179. Also, in case of splitting the image into sub images, the padding should contain the continued image data outside the cell. The new implementation uses considerations described in DM-28928.

             

            The variance plane calculation was algorithmically incorrect; the variance plane was subject to the very same calculations in the old implementation as the image plane (just with addition instead of subtraction). This is discussed in DMTN-179 Sec. 5.


            In addition to the issues originally raised in this ticket, we also considered the followings:

             

            In the spatially varying PSF case, the old implementation focused on splitting the image into individual small chunks and smoothing the solution by allowing and averaging overlapping borders back in image space. In the new implementation, we split the image into overlapping image chunks for the DFT calculations and keep only smaller “useful inner areas” that do not overlap to account for spatial variability of the PSFs.

             

            The old implementation added noise to low PSF values (in image space) to avoid numerical zero divisions in the calculations. We concluded that this approach did not solve any problem but added artificial noise to scenarios inherently noisy numerically. At close to zero denominators, the expressions should approach mathematical limits to reproduce the theoretically expected smooth solutions. This is discussed in DMTN-179.

             

            The old implementation unnecessarily implemented options to perform convolutions in image space rather than in DFT space. We concluded that this was not a cause of “bad” solutions.

             

            The old implementation did not create a spatially varying result PSF solution. The new implementation does, though the solution can “jump” between grid cells.

             

            The new implementation uses power of two dimensions for the padded (sub-)images for better FFT accuracy.

            Show
            gkovacs Gabor Kovacs [X] (Inactive) added a comment - The new implementation refers to the major zogy code rewrite in DM-25115 and its updates since then. The old implementation had a bug in how to center the PSF properly in an FFT symmetry sense ( DM-11990 ). The centering of the image before and after FFT transformations got unnecessarily much attention while it had no real importance. In the new implementation, we always put the image center to the DFT origin for better symmetry.   The image padding size got into focus at the beginning for the wrong reason. We displayed the zogy matching kernel (and decorrelation afterburner) kernels in image space and had the impression that they contain long tails stretching up to hundreds of pixels. Later it became clear that this was due to noise in high frequency components which fills the image space representations irrespectively of their padding size. Nevertheless, to avoid the introduction of artifacts due to the circular boundary condition of DFT space calculations, we need to estimate the matching kernel dimensions in image space. See the theoretical Gaussian solutions in DMTN-179. Also, in case of splitting the image into sub images, the padding should contain the continued image data outside the cell. The new implementation uses considerations described in DM-28928 .   The variance plane calculation was algorithmically incorrect; the variance plane was subject to the very same calculations in the old implementation as the image plane (just with addition instead of subtraction). This is discussed in DMTN-179 Sec. 5. In addition to the issues originally raised in this ticket, we also considered the followings:   In the spatially varying PSF case, the old implementation focused on splitting the image into individual small chunks and smoothing the solution by allowing and averaging overlapping borders back in image space. In the new implementation, we split the image into overlapping image chunks for the DFT calculations and keep only smaller “useful inner areas” that do not overlap to account for spatial variability of the PSFs.   The old implementation added noise to low PSF values (in image space) to avoid numerical zero divisions in the calculations. We concluded that this approach did not solve any problem but added artificial noise to scenarios inherently noisy numerically. At close to zero denominators, the expressions should approach mathematical limits to reproduce the theoretically expected smooth solutions. This is discussed in DMTN-179.   The old implementation unnecessarily implemented options to perform convolutions in image space rather than in DFT space. We concluded that this was not a cause of “bad” solutions.   The old implementation did not create a spatially varying result PSF solution. The new implementation does, though the solution can “jump” between grid cells.   The new implementation uses power of two dimensions for the padded (sub-)images for better FFT accuracy.
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April), AP F21-1 (June) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084, 1096 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April), AP F21-1 (June), AP F21-2 (July) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084, 1096, 1102 ]
            Hide
            sullivan Ian Sullivan added a comment -

            Thank you for your updates on this ticket. I think you can go ahead and close it now.

            Show
            sullivan Ian Sullivan added a comment - Thank you for your updates on this ticket. I think you can go ahead and close it now.
            sullivan Ian Sullivan made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            sullivan Ian Sullivan made changes -
            Rank Ranked higher
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April), AP F21-1 (June), AP F21-2 (July) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084, 1096, 1102 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April), AP F21-1 (June), AP F21-2 (July), AP F21-3 (August) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084, 1096, 1102, 1109 ]
            sullivan Ian Sullivan made changes -
            Sprint AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April), AP F21-1 (June), AP F21-2 (July), AP F21-3 (August) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084, 1096, 1102, 1109 ] AP S20-5 (April), AP S20-6 (May), AP F20-1 (June), AP F20-2 (July), AP F20-5 (October), AP F20-6 (November), AP S21-3 (February), AP S21-4 (March), AP S21-5 (April), AP F21-1 (June), AP F21-2 (July), AP F21-3 (August), AP F21-4 (September) [ 986, 987, 1019, 1025, 1046, 1052, 1072, 1079, 1084, 1096, 1102, 1109, 1113 ]
            sullivan Ian Sullivan made changes -
            Assignee Gabor Kovacs [X] [ gkovacs ] Ian Sullivan [ sullivan ]
            sullivan Ian Sullivan made changes -
            Epic Link DM-30435 [ 504823 ] DM-30503 [ 510162 ]
            sullivan Ian Sullivan made changes -
            Epic Link DM-30503 [ 510162 ] DM-30435 [ 504823 ]
            sullivan Ian Sullivan made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            Hide
            sullivan Ian Sullivan added a comment -

            Should have been marked Done in July

            Show
            sullivan Ian Sullivan added a comment - Should have been marked Done in July
            sullivan Ian Sullivan made changes -
            Starting 30/Aug/21

              People

              Assignee:
              sullivan Ian Sullivan
              Reporter:
              gkovacs Gabor Kovacs [X] (Inactive)
              Reviewers:
              Ian Sullivan
              Watchers:
              Gabor Kovacs [X] (Inactive), Ian Sullivan
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.