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

Make an attempt at fixing the plate scale issue in obs_subaru

    XMLWordPrintable

Details

    • Story
    • Status: Done
    • Resolution: Done
    • None
    • obs_subaru
    • None
    • 12
    • Per-CCD Sprint 1
    • Data Release Production

    Description

      For the work being done in DM-19943, we need to have the CCD plate scale description in the camera geometry to properly reflect the pixel-to-mm conversion. Currently (for historical reasons?), the obs_subaru package defines this transformation to be unity and this is a showstopper for using HSC data as a test data for validation of the work of DM-19943. While all cameras are in the process of being converted to YamlCamera and will use consistent/correct scaling (this is DM-19352 for obs_subaru), we would like to be able to move forward with this before that is all merged (HSC is otherwise the ideal test dataset for these purposes because we understand it well and the distortion model is well represented). This ticket is thus to make an attempt at fixing this units issue in the current state of the camera representation (slight emphasis on "attempt" as this might be more complicated/convoluted than one might naively picture and this should not turn into a huge time sink).

      Attachments

        Issue Links

          Activity

            No builds found.
            lauren Lauren MacArthur created issue -
            lauren Lauren MacArthur made changes -
            Field Original Value New Value
            Epic Link DM-19943 [ 306001 ]
            lauren Lauren MacArthur made changes -
            Link This issue relates to DM-19735 [ DM-19735 ]
            lauren Lauren MacArthur made changes -
            Link This issue relates to DM-19731 [ DM-19731 ]
            erykoff Eli Rykoff made changes -
            Link This issue relates to DM-17862 [ DM-17862 ]
            erykoff Eli Rykoff added a comment -

            Fixing this would also close DM-17862, which is a blocker for DM-16490, so I'm definitely rooting for this to be done!

            erykoff Eli Rykoff added a comment - Fixing this would also close DM-17862 , which is a blocker for DM-16490 , so I'm definitely rooting for this to be done!

            I've gotten DM-20154 to build through lsst_distrib, but ci_hsc is failing, likely because of this issue. Were you able to make any progress on it?

            Parejkoj John Parejko added a comment - I've gotten DM-20154 to build through lsst_distrib, but ci_hsc is failing, likely because of this issue. Were you able to make any progress on it?
            lauren Lauren MacArthur made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            lauren Lauren MacArthur made changes -

            Sort of.... I updated hsc’s camera.py to use the correct plateScale and all looks good in the un-distorted world, but I can’t seem to get the distortion right and I’m totally confused as to what/where it’s going wrong (noting that the coefficients for the distortion terms are “inverse” and the forward ones are computed using ast in ways I don’t understand).

            The original layout looks like (distortion correction - red lines - clearly being applied):

            The “new” one looks like (note the axis scale, now in mm, but distortion is not being corrected for properly):

            The first place I think I see something going wrong is when I try computing pixelToDistortedPixel using

            focalPlaneToFieldAngle = camera.getTransformMap().getTransform(cameraGeom.FOCAL_PLANE,
             cameraGeom.FIELD_ANGLE)
            pixelToFocalPlane = ccd.getTransform(cameraGeom.PIXELS, cameraGeom.FOCAL_PLANE)
            afwGeom.wcsUtils.computePixelToDistortedPixel(
             pixelToFocalPlane=pixelToFocalPlane,
             focalPlaneToFieldAngle=focalPlaneToFieldAngle)
            

            which I think is supposed to give me the distorted pixel value in the CCD pixel coords, which I think should be the same answer in both cases. The original version for CCD=0 has (0, 0) -> (493.56, -140.48) whereas in the updated camera.py I get (0, 0) -> (0.13354, -0.021414). Strangely enough, if I try messing around with the distortion coefficients, this doesn’t seem to change. Also, if I “force-feed” the originally computed distorted CCD pixel value to the new version’s pixelToFocalPlane.applyForward(afwGeom.Point2D(493.56, -140.48)), I do seem to get the correct distorted FocalPlane pixel…I’m confused!

            lauren Lauren MacArthur added a comment - Sort of.... I updated hsc’s camera.py to use the correct plateScale and all looks good in the un-distorted world, but I can’t seem to get the distortion right and I’m totally confused as to what/where it’s going wrong (noting that the coefficients for the distortion terms are “inverse” and the forward ones are computed using ast in ways I don’t understand). The original layout looks like (distortion correction - red lines - clearly being applied): The “new” one looks like (note the axis scale, now in mm, but distortion is not being corrected for properly): The first place I think I see something going wrong is when I try computing pixelToDistortedPixel using focalPlaneToFieldAngle = camera.getTransformMap().getTransform(cameraGeom.FOCAL_PLANE, cameraGeom.FIELD_ANGLE) pixelToFocalPlane = ccd.getTransform(cameraGeom.PIXELS, cameraGeom.FOCAL_PLANE) afwGeom.wcsUtils.computePixelToDistortedPixel( pixelToFocalPlane = pixelToFocalPlane, focalPlaneToFieldAngle = focalPlaneToFieldAngle) which I think is supposed to give me the distorted pixel value in the CCD pixel coords, which I think should be the same answer in both cases. The original version for CCD=0 has (0, 0) -> (493.56, -140.48) whereas in the updated camera.py I get (0, 0) -> (0.13354, -0.021414). Strangely enough, if I try messing around with the distortion coefficients, this doesn’t seem to change. Also, if I “force-feed” the originally computed distorted CCD pixel value to the new version’s pixelToFocalPlane.applyForward(afwGeom.Point2D(493.56, -140.48)) , I do seem to get the correct distorted FocalPlane pixel…I’m confused!
            Parejkoj John Parejko added a comment -

            You shouldn't have to go through all of that. If you do:

            transform = camera.getTransformMap().getTransform(cameraGeom.PIXELS, cameraGeom.FIELD_ANGLE)
            

            you should get the exact transform from pixels to field angle.

            What are you doing to update the plate scale in the camera geometry? My tests with cmorrison's HSC notebook from DM-20188 and my tests with ci_hsc showed that I needed to make two changes to camera.py: https://github.com/lsst/obs_subaru/commit/46ff04713fb76108a66dddc8156d2847c0ec5fc1

            Parejkoj John Parejko added a comment - You shouldn't have to go through all of that. If you do: transform = camera.getTransformMap().getTransform(cameraGeom.PIXELS, cameraGeom.FIELD_ANGLE) you should get the exact transform from pixels to field angle. What are you doing to update the plate scale in the camera geometry? My tests with cmorrison 's HSC notebook from DM-20188 and my tests with ci_hsc showed that I needed to make two changes to camera.py : https://github.com/lsst/obs_subaru/commit/46ff04713fb76108a66dddc8156d2847c0ec5fc1
            Parejkoj John Parejko added a comment -

            I think the main question we have to deal with is how to handle test_distortion.py. It has an option to write out the "correct" result (set SAVE_DATA=True), but I don't know how thoroughly validated the existing values are against the distEst HSC package mentioned in its docstring.

            Parejkoj John Parejko added a comment - I think the main question we have to deal with is how to handle test_distortion.py . It has an option to write out the "correct" result (set SAVE_DATA=True ), but I don't know how thoroughly validated the existing values are against the distEst HSC package mentioned in its docstring.

            So it seems that only two entries in camera.py are required to get the initial "from boresight" correctly set. However, this still leaves the camera definition inconsistent with how it is set in other obs packages in that all the individual detectors have their offset_x/y and pixelSize_x/y values in units of mm (whereas they are still in "pixels" for obs_subaru), so I'm a bit worried about potential gotchas we're not anticipating.

            lauren Lauren MacArthur added a comment - So it seems that only two entries in  camera.py are required to get the initial "from boresight" correctly set. However, this still leaves the camera definition inconsistent with how it is set in other obs packages in that all the individual detectors have their offset_x/y and pixelSize_x/y values in units of mm (whereas they are still in "pixels" for obs_subaru ), so I'm a bit worried about potential gotchas we're not anticipating.
            tjenness Tim Jenness made changes -
            Link This issue relates to DM-19352 [ DM-19352 ]
            lauren Lauren MacArthur made changes -
            Attachment showCameraNew.png [ 39034 ]
            lauren Lauren MacArthur made changes -
            Attachment showCameraNew.png [ 39034 ]
            lauren Lauren MacArthur made changes -
            Attachment showCameraNew.png [ 39035 ]
            lauren Lauren MacArthur made changes -
            Attachment deltaDistortionHist.png [ 39036 ]
            lauren Lauren MacArthur made changes -
            Attachment deltaDistortionVsCcdId.png [ 39037 ]

            Being unsatisfied with the level of inconsistencies in the camera definitions the way we left it above, I looked more deeply into the distortion coefficients and how they were derived and being applied. It turns out they needed a somewhat complicated scaling to get them into coordinates using the proper physical (as opposed to the HSC "unity") scaling. See changes and commentary in the file transformRegistry.py in this commit.

            Using this scaling for makeHscDistortion, we can now consistently refer to all camera and detector parameters in camera.py in their physical units (namely plateScale in arcsec/mm, and offset_x/y and pixelSize_x/y in mm, from which we can derive the {pixelScale = plateScale * pixelSize}} in arcsec/pixel.

            I can now get the proper distortion plotted in Focal Plane units of mm. I have updated the script in bin.src/showCamera.py to produce the following:

            which looks identical to the original but for the "pixel" to "mm" units. Just to be sure, I did compare the old vs. new distorted pixel positions (in ccd pixels). There are small differences with the maximum being ~0.27 pixels (which could boil down to machine epsilon seeing as many of the coefficients are at the e-38 level!). Here is what the distributions look like as histograms and as a function of ccdId (see other plots above for the ccdId-labeled layout):


            On the face of it, I believe these are small enough that we need not worry about them.

            lauren Lauren MacArthur added a comment - Being unsatisfied with the level of inconsistencies in the camera definitions the way we left it above, I looked more deeply into the distortion coefficients and how they were derived and being applied. It turns out they needed a somewhat complicated scaling to get them into coordinates using the proper physical (as opposed to the HSC "unity") scaling. See changes and commentary in the file transformRegistry.py in this commit . Using this scaling for makeHscDistortion , we can now consistently refer to all camera and detector parameters in camera.py in their physical units (namely plateScale in arcsec/mm, and offset_x/y and pixelSize_x/y in mm, from which we can derive the {pixelScale = plateScale * pixelSize}} in arcsec/pixel. I can now get the proper distortion plotted in Focal Plane units of mm. I have updated the script in bin.src/showCamera.py to produce the following: which looks identical to the original but for the "pixel" to "mm" units. Just to be sure, I did compare the old vs. new distorted pixel positions (in ccd pixels). There are small differences with the maximum being ~0.27 pixels (which could boil down to machine epsilon seeing as many of the coefficients are at the e-38 level!). Here is what the distributions look like as histograms and as a function of ccdId (see other plots above for the ccdId-labeled layout): On the face of it, I believe these are small enough that we need not worry about them.

            Ah, I just saw that my Jenkins run had a failure in a meas_mosaic test.  This is not surprising as I'm sure that meas_mosaic also works in the context of the "unity" scaling.  We should discuss if this is worth fixing, but I'll have a quick look now to see if it looks like an easy-ish fix.

            lauren Lauren MacArthur added a comment - Ah, I just saw that my Jenkins run had a failure in a meas_mosaic test.  This is not surprising as I'm sure that meas_mosaic also works in the context of the "unity" scaling.  We should discuss if this is worth fixing, but I'll have a quick look now to see if it looks like an easy-ish fix.

            I have also confirmed the current branch fixes erykoff's issue reported on DM-17862.

            lauren Lauren MacArthur added a comment - I have also confirmed the current branch fixes erykoff 's issue reported on DM-17862 .
            erykoff Eli Rykoff added a comment -

            This all looks great! And brings up the question of when we're officially retiring meas_mosaic. I'm also wondering if this fix breaks some qa plots in fgcmcal (which wouldn't show up in tests).

            erykoff Eli Rykoff added a comment - This all looks great! And brings up the question of when we're officially retiring meas_mosaic . I'm also wondering if this fix breaks some qa plots in fgcmcal (which wouldn't show up in tests).
            price Paul Price added a comment -

            Good onya, Loza!

            Well done.

            P.

            price Paul Price added a comment - Good onya, Loza! Well done. P.

            Thanks Paul.  Any thoughts (wearing your NAOJ hat) on making sure meas_mosaic also gets properly converted?

            lauren Lauren MacArthur added a comment - Thanks Paul.  Any thoughts (wearing your NAOJ hat) on making sure meas_mosaic also gets properly converted?
            price Paul Price added a comment -

            Ack, that was meant to be a private comment, but I guess I don't mind if everyone sees it!

            We don't want to decommission meas_mosaic without first discussing with Japan, but my impression is that since we've been leaning on jointcal for the latest production run, meas_mosaic will no longer be needed. However, I would feel better about it if jointcal was much faster.

            price Paul Price added a comment - Ack, that was meant to be a private comment, but I guess I don't mind if everyone sees it! We don't want to decommission meas_mosaic without first discussing with Japan, but my impression is that since we've been leaning on jointcal for the latest production run, meas_mosaic will no longer be needed. However, I would feel better about it if jointcal was much faster.
            lauren Lauren MacArthur made changes -
            Link This issue relates to DM-20548 [ DM-20548 ]

            Ok, besides meas_mosaic, I think all issues with the move to define the focal plane camera geometry of HSC in units of mm have been resolved.  The major changes were as following:

            • updating camera.py to use mm units in the top level and transform configs as well as for each individual detector
            • scaling of the distortion model coefficients (which were derived in the context of "unity" scaling) to that of mm. This is done in makeAstPolyMapCoeffs() in hoc's transformRegistry.py
            • update all config overrides that were using "unity" units (includes isr.py, vignette.py, focalBackground.py, and skyCorr.py)

            The following packages also needed updating to adapt to the new units:
            pipe_drivers, fgcmcal, and meas_mosaic (the latter just to get the tests passing). In the case of pipe_drivers, it had attempted to account for focal planes being defined in different units for different cameras, but it was missing one scaling that set an important threshold. With the fix here, the derived {{skyCorr}}s are now identical for the old vs. new unit systems.

            There are tickets/DM-20289 branches that are compatible with the tickets/DM-20154 branches. None of the tickets/DM-20289 will be merged (see below), but they may be useful for any further work on DM-20154 with HSC data. As such, the work for this ticket is effectively completed.

            The actual switch to the new mm unit focal plane scale for HSC is going to happen on a separate ticket (DM-20548) that will be isolated from the work on DM-20154 (and will have to go through an RFC process first). Further battle testing will also be done on at least one full tract from the RC2 dataset (i.e. running through singleFrameDriver, jointcal, coaddDriver, and multibandDriver).

            lauren Lauren MacArthur added a comment - Ok, besides meas_mosaic , I think all issues with the move to define the focal plane camera geometry of HSC in units of mm have been resolved.  The major changes were as following: updating  camera.py to use mm units in the top level and transform configs as well as for each individual detector scaling of the distortion model coefficients (which were derived in the context of "unity" scaling) to that of mm. This is done in makeAstPolyMapCoeffs() in hoc's transformRegistry.py update all config overrides that were using "unity" units (includes isr.py , vignette.py , focalBackground.py , and skyCorr.py ) The following packages also needed updating to adapt to the new units: pipe_drivers , fgcmcal , and meas_mosaic (the latter just to get the tests passing). In the case of pipe_drivers , it had attempted to account for focal planes being defined in different units for different cameras, but it was missing one scaling that set an important threshold. With the fix here, the derived {{skyCorr}}s are now identical for the old vs. new unit systems. There are tickets/ DM-20289 branches that are compatible with the tickets/ DM-20154 branches. None of the tickets/ DM-20289 will be merged (see below), but they may be useful for any further work on DM-20154 with HSC data. As such, the work for this ticket is effectively completed. The actual switch to the new mm unit focal plane scale for HSC is going to happen on a separate ticket ( DM-20548 ) that will be isolated from the work on DM-20154 (and will have to go through an RFC process first). Further battle testing will also be done on at least one full tract from the RC2 dataset (i.e. running through singleFrameDriver , jointcal , coaddDriver , and multibandDriver ).
            lauren Lauren MacArthur made changes -
            Link This issue relates to DM-20548 [ DM-20548 ]
            lauren Lauren MacArthur made changes -
            Story Points 12

            Would you mind having a look and confirming if the work done here is sufficient to close this out?

            lauren Lauren MacArthur added a comment - Would you mind having a look and confirming if the work done here is sufficient to close this out?
            lauren Lauren MacArthur made changes -
            Reviewers Jim Bosch [ jbosch ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            lauren Lauren MacArthur added a comment - - edited

            Oh, and a final Jenkins with:

            PRODUCTS: lsst_distrib lsst_ci ci_hsc
            REFS: tickets/DM-20289 tickets/DM-20154
            

            is green.

            lauren Lauren MacArthur added a comment - - edited Oh, and a final Jenkins with: PRODUCTS: lsst_distrib lsst_ci ci_hsc REFS: tickets/DM-20289 tickets/DM-20154 is green .
            yusra Yusra AlSayyad made changes -
            Sprint Per-CCD Sprint 1 [ 919 ] DRP F19-2 [ 939 ]
            yusra Yusra AlSayyad made changes -
            Sprint DRP F19-2 [ 939 ] Per-CCD Sprint 1 [ 919 ]
            jbosch Jim Bosch made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            lauren Lauren MacArthur made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            yusra Yusra AlSayyad made changes -
            Link This issue is contained by DM-21280 [ DM-21280 ]
            yusra Yusra AlSayyad made changes -
            Team Data Release Production [ 10301 ]
            yusra Yusra AlSayyad made changes -
            Team Data Release Production [ 10301 ]
            yusra Yusra AlSayyad made changes -
            Team Data Release Production [ 10301 ]
            swinbank John Swinbank made changes -
            Team Data Release Production [ 10301 ]
            yusra Yusra AlSayyad made changes -
            Team Data Release Production [ 10301 ]
            yusra Yusra AlSayyad made changes -
            Team Data Release Production [ 10301 ]
            yusra Yusra AlSayyad made changes -
            Link This issue is contained by DM-22452 [ DM-22452 ]
            yusra Yusra AlSayyad made changes -
            Link This issue is contained by DM-22452 [ DM-22452 ]
            yusra Yusra AlSayyad made changes -
            Team Data Release Production [ 10301 ]
            yusra Yusra AlSayyad made changes -
            Team Data Release Production [ 10301 ]
            yusra Yusra AlSayyad made changes -
            Team Data Release Production [ 10301 ]
            yusra Yusra AlSayyad made changes -
            Team Data Release Production [ 10301 ]

            People

              lauren Lauren MacArthur
              lauren Lauren MacArthur
              Jim Bosch
              Chris Morrison [X] (Inactive), Eli Rykoff, Jim Bosch, John Parejko, John Swinbank, Lauren MacArthur, Paul Price
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.