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

Possible inconsistency in indexing in the brighter fatter kernel generation/correction

    XMLWordPrintable

Details

    • Bug
    • Status: Done
    • Resolution: Done
    • None
    • cp_pipe, ip_isr
    • None
    • 4
    • Data Release Production
    • No

    Description

      Looking at PhotonTransferCurveDataset.aMatrix below, it has a higher correlation along with the horizontal direction. It should be in the vertical direction to be matched with the LSST sensor orientation. This finding triggered all the slack discussions attached to this ticket. 

       

      abrought said

      Hi Chris, I was going over the quality of the BF correction, and noticed that it was doing ok, but not great.  So I was trying to cross off some potential sources of error. One route I was considering was that the applied kernel was “flipped” due to difference in the image coordinate system and the numpy ndarray indexing. I know that the real pixels have a lower confining field in vertical, or parallel direction, and so the A matrix component in the vertical direction tends to be larger than it is in the horizontal direction, but if it is not applied in the right orientation, it could be causing the results I’m seeing. I was using fiducial behavior to gauge the difference in image coordinates and array indexing.

      Below I’ve plotted the A matrix and Kernel[8:, 8:] matrix, with the indexing (happy to give you the code too). And I noticed that the sensor “parallel” direction looks horizontal on these plots, which seems wrong to me, but might be right with respect to the image coordinate system. I’ve spent a while looking at the indexing here, and I think I might be going crazy, but since you wrote most of the BF code, I thought I’d ask you if this is correct.

      Thanks! And sorry about this, I know DM people are very busy right now.

      czw replied

      Can you tell me what config options you've used?  I see a comment on line 412 in the quadratic fit code that the i,j indices are inverted to apply the transposition, as is done in the averaging case.  I can see there's a transpose in the averageCorrelations function (which I'm a bit concerned is not doing the right thing either, as that looks like it needs an axes argument to transpose the correlations individually).

      One thing that would be useful to check (generally; this probably should be a unit test) would be to make a PTC dataset that has obvious covariance along one direction only, and confirm that the generated BFK corrects it.  This would test both the generation (is that missing axes actually a problem) and application (is the kernel in numpy coordinates and not afw image coordinates).

      As I'm tracing it, makeBrighterFatterKernel takes the PTC covariances, generates a BFK from the transposes (I don't remember why, I'll have to reread the paper) and saves that to disk (either on the AMP or DETECTOR levels).  IsrTask reads that in, using the detKernel directly if it exists, otherwise generating an average from the ampKernel entries.  That averaging again transposes the list of amp-wise kernels (with no axes), so AMP and DETECTOR level products are already different here, meaning there's at least one bug.  This numpy array is then packed into an afwImage's array, which should be transposed relative to the image coordinates (meaning a properly made kernel needs to be transposed once more, if it wasn't already transposed before).

      In a separate thread, czw added links to the possible suspected places:

      Attachments

        1. bfk_math-20230505.ipynb
          32 kB
          Christopher Waters
        2. BFKnotebook.ipynb
          9.44 MB
          Yousuke Utsumi
        3. download-39.png
          490 kB
          Yousuke Utsumi
        4. Screen Shot 2023-04-21 at 2.54.43 PM.png
          102 kB
          Yousuke Utsumi
        5. Screen Shot 2023-04-21 at 9.41.15 AM (1).png
          382 kB
          Yousuke Utsumi
        6. Screen Shot 2023-05-11 at 3.44.54 PM.png
          310 kB
          Alex Broughton
        7. Screen Shot 2023-05-15 at 12.13.43 PM.png
          1.39 MB
          Alex Broughton
        8. Screen Shot 2023-05-15 at 12.15.22 PM.png
          1.38 MB
          Alex Broughton
        9. Screen Shot 2023-05-15 at 12.15.32 PM.png
          592 kB
          Alex Broughton

        Activity

          abrought Alex Broughton added a comment - - edited

          I have independently tested the tickets/DM-38827 branch and can confirm that it gives the correct corrections and everything is in proper orientation.

          Below is a plot of an amplifier kernel generated with this branch, showing the slices in numpy array orientation [y,x].
          We expect that the A matrix is larger in the parallel direction away from the central pixel. The A matrix is the gradient of the kernel, and you can see that the gradient is larger moving away from the central pixel in the y slice than in the x slice.

          {code:python}# In /repo/main. . . bfk = butler.get('bfk', detector=23, instrument='LSSTCam', collections='u/abrought/BF/2023.04.28/bfk.2023.04.28.R03-S12.trunc_to_pcti.amatrix.improved_corr_model.noForceZeroSum.chrisfixes')
           
          fig, axs = plt.subplots(2,8, sharex=True, sharey=True,figsize=(20,8))
          for i in range(16):
              
              channel = slacAmps2[f"AMP{i+1:02d}"]
              ax = axs.flatten()[i]
              ax.axhline(0.0, linestyle="--", color="gray", linewidth=1)
              #print(channel, bfk.gain[channel], ptc.gain[channel])
              ax.plot(bfk.ampKernels[channel][:,8], drawstyle='steps-mid', label="x slice")
              ax.plot(bfk.ampKernels[channel][8,:], drawstyle='steps-mid', label="y slice")
              ax.set_title(f"AMP {i+1:02d}/{channel}")
              if channel == "C04":
                  ax.legend()

           

          abrought Alex Broughton added a comment - - edited I have independently tested the tickets/ DM-38827 branch and can confirm that it gives the correct corrections and everything is in proper orientation. Below is a plot of an amplifier kernel generated with this branch, showing the slices in numpy array orientation [y,x] . We expect that the A matrix is larger in the parallel direction away from the central pixel. The A matrix is the gradient of the kernel, and you can see that the gradient is larger moving away from the central pixel in the y slice than in the x slice. {code:python}# In /repo/main. . . bfk = butler.get( 'bfk' , detector= 23 , instrument= 'LSSTCam' , collections= 'u/abrought/BF/2023.04.28/bfk.2023.04.28.R03-S12.trunc_to_pcti.amatrix.improved_corr_model.noForceZeroSum.chrisfixes' )   fig, axs = plt.subplots( 2 , 8 , sharex=True, sharey=True,figsize=( 20 , 8 )) for i in range( 16 ): channel = slacAmps2[f "AMP{i+1:02d}" ] ax = axs.flatten()[i] ax.axhline( 0.0 , linestyle= "--" , color= "gray" , linewidth= 1 ) #print(channel, bfk.gain[channel], ptc.gain[channel]) ax.plot(bfk.ampKernels[channel][:, 8 ], drawstyle= 'steps-mid' , label= "x slice" ) ax.plot(bfk.ampKernels[channel][ 8 ,:], drawstyle= 'steps-mid' , label= "y slice" ) ax.set_title(f "AMP {i+1:02d}/{channel}" ) if channel == "C04" : ax.legend()  

          Just to double check again, I ran with Chris's fixes and Chris's fixes with the transposed kernel, and the one with Chris's fixes performed better, which tells me that the orientations with the new code updates. Phew!

           

          With Chris's Fixes: 

          With Chris's fixes + transposed kernel:

          abrought Alex Broughton added a comment - Just to double check again, I ran with Chris's fixes and Chris's fixes with the transposed kernel, and the one with Chris's fixes performed better, which tells me that the orientations with the new code updates. Phew!   With Chris's Fixes:  With Chris's fixes + transposed kernel:
          czw Christopher Waters added a comment - https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/38772/pipeline

          Please note that the plots above have implemented new fudge factor. We’ll give you more information later but this is likely what we need to implement.

          youtsumi Yousuke Utsumi added a comment - Please note that the plots above have implemented new fudge factor. We’ll give you more information later but this is likely what we need to implement.

          Nothing to add on PRs, changes look great, thanks!

          mfisherlevine Merlin Fisher-Levine added a comment - Nothing to add on PRs, changes look great, thanks!

          People

            czw Christopher Waters
            youtsumi Yousuke Utsumi
            Merlin Fisher-Levine
            Alex Broughton, Andrés Alejandro Plazas Malagón, Christopher Waters, Eli Rykoff, Merlin Fisher-Levine, Yousuke Utsumi
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Jenkins

                No builds found.