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

Correct for distortion in matchOptimisticB astrometry matcher

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_astrom, obs_subaru
    • Labels:
      None

      Description

      matchOptimisticB does not correct for distortion, although an estimate of the distortion is available. We suspect that doing the matching on the celestial sphere might be ideal, but matching on a tangent plane has worked for HSC.

        Attachments

          Issue Links

            Activity

            Hide
            price Paul Price added a comment -

            Simon Krughoff, would you mind reviewing this, please?

            price@price-laptop:~/LSST/meas/astrom (tickets/DM-3492=) $ git sub
            commit c84501f95b7899ddeb4b8b3ee13d361e9d0ae7cd
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Tue Aug 18 15:10:25 2015 -0700
             
                matchOptimisticB: correct for distortion
                
                We use the estimated distortion (in the form of a DistortedTanWcs)
                to correct the source positions.  Both distortion-corrected source
                and reference sky coordinates are projected using a simple linear
                Wcs, and an offset applied to bring them as close together as
                possible.
             
             include/lsst/meas/astrom/matchOptimisticB.h | 10 ++++--
             python/lsst/meas/astrom/matchOptimisticB.py |  1 +
             src/matchOptimisticB.cc                     | 54 ++++++++++++++++++++++++-----
             3 files changed, 55 insertions(+), 10 deletions(-)
             
             
            price@price-laptop:~/LSST/obs/subaru (tickets/DM-3492=) $ git sub
            commit b07775529420ed41c0ce8db97e5908c947cc287b
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Wed Aug 19 00:29:57 2015 -0400
             
                config: fix filterMap to use new names
                
                The solver has been split, and the filterMap is now under
                'refObjLoader'.
             
             config/processCcd.py | 4 ++--
             1 file changed, 2 insertions(+), 2 deletions(-)
             
            commit fdd0416ea5b972775db1b70b8b9c94a46bd70362
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Wed Aug 19 00:31:46 2015 -0400
             
                config: set match offset to allow edge CCDs to solve
                
                Now that we're dealing with distortion in the optimisticMatchB
                algorithm, we need a larger offset for the solution.
             
             config/hsc/processCcd.py | 1 +
             1 file changed, 1 insertion(+)
            

            Show
            price Paul Price added a comment - Simon Krughoff , would you mind reviewing this, please? price@price-laptop:~/LSST/meas/astrom (tickets/DM-3492=) $ git sub commit c84501f95b7899ddeb4b8b3ee13d361e9d0ae7cd Author: Paul Price <price@astro.princeton.edu> Date: Tue Aug 18 15:10:25 2015 -0700   matchOptimisticB: correct for distortion We use the estimated distortion (in the form of a DistortedTanWcs) to correct the source positions. Both distortion-corrected source and reference sky coordinates are projected using a simple linear Wcs, and an offset applied to bring them as close together as possible.   include/lsst/meas/astrom/matchOptimisticB.h | 10 ++++-- python/lsst/meas/astrom/matchOptimisticB.py | 1 + src/matchOptimisticB.cc | 54 ++++++++++++++++++++++++----- 3 files changed, 55 insertions(+), 10 deletions(-)     price@price-laptop:~/LSST/obs/subaru (tickets/DM-3492=) $ git sub commit b07775529420ed41c0ce8db97e5908c947cc287b Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 19 00:29:57 2015 -0400   config: fix filterMap to use new names The solver has been split, and the filterMap is now under 'refObjLoader'.   config/processCcd.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)   commit fdd0416ea5b972775db1b70b8b9c94a46bd70362 Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 19 00:31:46 2015 -0400   config: set match offset to allow edge CCDs to solve Now that we're dealing with distortion in the optimisticMatchB algorithm, we need a larger offset for the solution.   config/hsc/processCcd.py | 1 + 1 file changed, 1 insertion(+)
            Hide
            krughoff Simon Krughoff added a comment -

            No problem. I'll do it today.

            Show
            krughoff Simon Krughoff added a comment - No problem. I'll do it today.
            Hide
            krughoff Simon Krughoff added a comment - - edited

            So this looks fine to me. I would really like to see an update of the unit test to show that this works in regions of higher distortion than it did before. If that can't happen as part of this ticket, can you please file an issue to capture that technical debt?

            Edit: I meant distortion not density

            Show
            krughoff Simon Krughoff added a comment - - edited So this looks fine to me. I would really like to see an update of the unit test to show that this works in regions of higher distortion than it did before. If that can't happen as part of this ticket, can you please file an issue to capture that technical debt? Edit: I meant distortion not density
            Hide
            price Paul Price added a comment -

            Do you mean a unit test with distortion? I don't understand what higher density has to do with this issue.

            Show
            price Paul Price added a comment - Do you mean a unit test with distortion? I don't understand what higher density has to do with this issue.
            Hide
            price Paul Price added a comment -

            Simon Krughoff found some more problems, so taking this back until it's working properly.

            Show
            price Paul Price added a comment - Simon Krughoff found some more problems, so taking this back until it's working properly.
            Hide
            price Paul Price added a comment -

            In response to Simon Krughoff's problems, I've improved the fitting (including the addition of rejection iterations, a long-desired feature) and fixed a few other things I found (including a small bug in the matching). I've revised the original distortion solution slightly, and added a unit test (thanks for insisting on this — it was a helpful exercise). I've successfully run all 104 of the CCDs in the visit Simon was having problems with. Simon Krughoff, back to you if you don't mind. Comments from other interested parties are also welcome.

            Here's the revised set of commits:

            pprice@tiger-sumire:~/LSST/meas/astrom (tickets/DM-3492=) $ git sub
            commit 8c738b7e4b9f2b01a240f91beab59b5e711e72b1
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Tue Aug 18 15:10:25 2015 -0700
             
                matchOptimisticB: correct for distortion
                
                We use the estimated distortion (in the form of a DistortedTanWcs)
                to correct the source positions.  Both distortion-corrected source
                and reference sky coordinates are projected using a simple linear
                Wcs.
             
             include/lsst/meas/astrom/matchOptimisticB.h | 10 ++++-
             python/lsst/meas/astrom/matchOptimisticB.py |  1 +
             src/matchOptimisticB.cc                     | 59 +++++++++++++++++++++++++----
             3 files changed, 60 insertions(+), 10 deletions(-)
             
            commit 27a5b016cb3a42ca5ebe1617620495b06d73d8d5
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Wed Aug 19 21:56:41 2015 -0700
             
                FitTanSipWcsTask: generate guess Wcs before fitting
                
                The input Wcs we've been given is a wild guess, but matching
                doesn't provide a Wcs we can use as our initial guess for fitting.
                We generate the guess Wcs from the match list and the pixel scale
                from the input Wcs.
             
             python/lsst/meas/astrom/fitTanSipWcs.py | 24 +++++++++++++++++++++++-
             1 file changed, 23 insertions(+), 1 deletion(-)
             
            commit 44a6fe64eed5c51be0f5d53f01a15df02895c59b
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Wed Aug 19 22:01:19 2015 -0700
             
                FitTanSipWcsTask: add rejection and visualisation
                
                We can get some rogue matches which should be rejected as part
                of the Wcs fitting (but that's not done in CreateWcsWithSip).
                To help visualise the rejection, added some plots of the
                residuals.
             
             python/lsst/meas/astrom/fitTanSipWcs.py | 101 +++++++++++++++++++++++++++++++-
             tests/testAstrometryTask.py             |   1 +
             2 files changed, 99 insertions(+), 3 deletions(-)
             
            commit 01534088e32d62153b98e920368517826a3a7fff
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Wed Aug 19 22:05:48 2015 -0700
             
                matchOptimisticB: fix use of allowedNonperpDeg
                
                allowedNonperpDeg was converted to radians, but then compared
                against a number that is in degrees.  Removed the radians
                version.
             
             src/matchOptimisticB.cc | 3 +--
             1 file changed, 1 insertion(+), 2 deletions(-)
             
            commit f7e5a0130f6dedc5b4c18c32df5e099fd0ddb667
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Wed Aug 19 22:07:34 2015 -0700
             
                matchOptimisticB: add explanation of debug values
             
             src/matchOptimisticB.cc | 3 +++
             1 file changed, 3 insertions(+)
             
            commit 9a5ccd3e9fdc32d050fd6bfba965b32345fbf455
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Wed Aug 19 22:08:43 2015 -0700
             
                add test for matchOptimisticB under strong distortion
             
             tests/testMatchOptimisticB.py | 44 +++++++++++++++++++++++++++++++++++++++++--
             1 file changed, 42 insertions(+), 2 deletions(-)
             
            commit f27515cccf35f41be58e7ca8b36eea3d814cc17d
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Thu Aug 20 01:42:11 2015 -0400
             
                matchOptimisticB: fix typo in log message
             
             python/lsst/meas/astrom/matchOptimisticB.py | 2 +-
             1 file changed, 1 insertion(+), 1 deletion(-)
            

            Show
            price Paul Price added a comment - In response to Simon Krughoff 's problems, I've improved the fitting (including the addition of rejection iterations, a long-desired feature) and fixed a few other things I found (including a small bug in the matching). I've revised the original distortion solution slightly, and added a unit test (thanks for insisting on this — it was a helpful exercise). I've successfully run all 104 of the CCDs in the visit Simon was having problems with. Simon Krughoff , back to you if you don't mind. Comments from other interested parties are also welcome. Here's the revised set of commits: pprice@tiger-sumire:~/LSST/meas/astrom (tickets/DM-3492=) $ git sub commit 8c738b7e4b9f2b01a240f91beab59b5e711e72b1 Author: Paul Price <price@astro.princeton.edu> Date: Tue Aug 18 15:10:25 2015 -0700   matchOptimisticB: correct for distortion We use the estimated distortion (in the form of a DistortedTanWcs) to correct the source positions. Both distortion-corrected source and reference sky coordinates are projected using a simple linear Wcs.   include/lsst/meas/astrom/matchOptimisticB.h | 10 ++++- python/lsst/meas/astrom/matchOptimisticB.py | 1 + src/matchOptimisticB.cc | 59 +++++++++++++++++++++++++---- 3 files changed, 60 insertions(+), 10 deletions(-)   commit 27a5b016cb3a42ca5ebe1617620495b06d73d8d5 Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 19 21:56:41 2015 -0700   FitTanSipWcsTask: generate guess Wcs before fitting The input Wcs we've been given is a wild guess, but matching doesn't provide a Wcs we can use as our initial guess for fitting. We generate the guess Wcs from the match list and the pixel scale from the input Wcs.   python/lsst/meas/astrom/fitTanSipWcs.py | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)   commit 44a6fe64eed5c51be0f5d53f01a15df02895c59b Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 19 22:01:19 2015 -0700   FitTanSipWcsTask: add rejection and visualisation We can get some rogue matches which should be rejected as part of the Wcs fitting (but that's not done in CreateWcsWithSip). To help visualise the rejection, added some plots of the residuals.   python/lsst/meas/astrom/fitTanSipWcs.py | 101 +++++++++++++++++++++++++++++++- tests/testAstrometryTask.py | 1 + 2 files changed, 99 insertions(+), 3 deletions(-)   commit 01534088e32d62153b98e920368517826a3a7fff Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 19 22:05:48 2015 -0700   matchOptimisticB: fix use of allowedNonperpDeg allowedNonperpDeg was converted to radians, but then compared against a number that is in degrees. Removed the radians version.   src/matchOptimisticB.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)   commit f7e5a0130f6dedc5b4c18c32df5e099fd0ddb667 Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 19 22:07:34 2015 -0700   matchOptimisticB: add explanation of debug values   src/matchOptimisticB.cc | 3 +++ 1 file changed, 3 insertions(+)   commit 9a5ccd3e9fdc32d050fd6bfba965b32345fbf455 Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 19 22:08:43 2015 -0700   add test for matchOptimisticB under strong distortion   tests/testMatchOptimisticB.py | 44 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-)   commit f27515cccf35f41be58e7ca8b36eea3d814cc17d Author: Paul Price <price@astro.princeton.edu> Date: Thu Aug 20 01:42:11 2015 -0400   matchOptimisticB: fix typo in log message   python/lsst/meas/astrom/matchOptimisticB.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
            Hide
            price Paul Price added a comment -

            [11:04 AM] Jenkins: Completed #3231 branch: tickets/DM-3492 of lsst_sims lsst_distrib (centos-6) for Paul Price Success after 9 min 54 sec (Job) (Console)
            [11:04 AM] Jenkins: Completed #3231 branch: tickets/DM-3492 of lsst_sims lsst_distrib (centos-7) for Paul Price Success after 10 min (Job) (Console)

            Show
            price Paul Price added a comment - [11:04 AM] Jenkins: Completed #3231 branch: tickets/ DM-3492 of lsst_sims lsst_distrib (centos-6) for Paul Price Success after 9 min 54 sec (Job) (Console) [11:04 AM] Jenkins: Completed #3231 branch: tickets/ DM-3492 of lsst_sims lsst_distrib (centos-7) for Paul Price Success after 10 min (Job) (Console)
            Hide
            rowen Russell Owen added a comment - - edited

            This looks great to me. You made some very nice and well explained enhancements. Thank you for adding outlier rejection and fixing my bug in handling of allowedNonperpDeg.

            How crucial is it that matchOptimisticB be runnable without a WCS? If we have no firm use case for this then I suggest making the WCS a required argument (and a reference instead of a pointer). Internally handle distortion only if the WCS has distortion terms. However, always use the WCS to compute centroids for reference objects. The advantages that I see are:

            • The API is more uniform and easier to explain to users, admittedly at the expense of requiring an argument that was formerly optional (and in the case of no distortion still could be optional)
            • The reference catalog source positions are computed internally, and in C++, making the subroutine easier to call correctly for users, and also a bit faster

            Two minor nit-picks in matchOptimisticB.cc:

            In _fitWcs why do you call "wcs = sipObject.getNewWcs()"? The value is immediately thrown away, so this will make linters complain. If you do need to make that call for some reason (e.g. to force an internal computation in sipObject) then I suggest that you offer a brief comment as to why the call is necessary, and also discard the result rather than assigning it to a variable.

            Would it make more sense to compute allowedNonperpRad and compute the value in radians? Our code is supposed to use angles in radians unless otherwise noted (and this does happen in a loop). If you keep the current algorithm, please consider renaming theta to thetaDeg to make bugs such as mine less likely in the future.

            Show
            rowen Russell Owen added a comment - - edited This looks great to me. You made some very nice and well explained enhancements. Thank you for adding outlier rejection and fixing my bug in handling of allowedNonperpDeg. How crucial is it that matchOptimisticB be runnable without a WCS? If we have no firm use case for this then I suggest making the WCS a required argument (and a reference instead of a pointer). Internally handle distortion only if the WCS has distortion terms. However, always use the WCS to compute centroids for reference objects. The advantages that I see are: The API is more uniform and easier to explain to users, admittedly at the expense of requiring an argument that was formerly optional (and in the case of no distortion still could be optional) The reference catalog source positions are computed internally, and in C++, making the subroutine easier to call correctly for users, and also a bit faster Two minor nit-picks in matchOptimisticB.cc: In _fitWcs why do you call "wcs = sipObject.getNewWcs()"? The value is immediately thrown away, so this will make linters complain. If you do need to make that call for some reason (e.g. to force an internal computation in sipObject) then I suggest that you offer a brief comment as to why the call is necessary, and also discard the result rather than assigning it to a variable. Would it make more sense to compute allowedNonperpRad and compute the value in radians? Our code is supposed to use angles in radians unless otherwise noted (and this does happen in a loop). If you keep the current algorithm, please consider renaming theta to thetaDeg to make bugs such as mine less likely in the future.
            Hide
            price Paul Price added a comment -

            I propose to take Russell Owen's comments as the review. Is this OK with you, Simon Krughoff?

            Show
            price Paul Price added a comment - I propose to take Russell Owen 's comments as the review. Is this OK with you, Simon Krughoff ?
            Hide
            price Paul Price added a comment -

            I will update the matchOptimisticB API to require a Wcs.

            In _fitWcs, the wcs is updated for the next iteration (note that the call is in a loop).

            I'll modify the code to use Angle instead of implicitly assuming degrees.

            Show
            price Paul Price added a comment - I will update the matchOptimisticB API to require a Wcs . In _fitWcs , the wcs is updated for the next iteration (note that the call is in a loop). I'll modify the code to use Angle instead of implicitly assuming degrees.
            Hide
            price Paul Price added a comment -

            I've pushed two fixup commits for review:

            commit 6f741decada4b41983ba25d1ca17a12c47231897
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Wed Aug 26 12:55:31 2015 -0400
             
                fixup! matchOptimisticB: correct for distortion
             
             include/lsst/meas/astrom/matchOptimisticB.h |  8 +++----
             src/matchOptimisticB.cc                     | 36 +++++++++--------------------
             2 files changed, 15 insertions(+), 29 deletions(-)
             
            commit c9ac6fecbe4d86ab9b8f305dfe2d62089b66fd4f
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Wed Aug 26 12:55:50 2015 -0400
             
                fixup! matchOptimisticB: fix use of allowedNonperpDeg
             
             src/matchOptimisticB.cc | 9 +++++----
             1 file changed, 5 insertions(+), 4 deletions(-)
            

            Show
            price Paul Price added a comment - I've pushed two fixup commits for review: commit 6f741decada4b41983ba25d1ca17a12c47231897 Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 26 12:55:31 2015 -0400   fixup! matchOptimisticB: correct for distortion   include/lsst/meas/astrom/matchOptimisticB.h | 8 +++---- src/matchOptimisticB.cc | 36 +++++++++-------------------- 2 files changed, 15 insertions(+), 29 deletions(-)   commit c9ac6fecbe4d86ab9b8f305dfe2d62089b66fd4f Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 26 12:55:50 2015 -0400   fixup! matchOptimisticB: fix use of allowedNonperpDeg   src/matchOptimisticB.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
            Hide
            rowen Russell Owen added a comment -

            Many thanks for the enhancements. Changing theta to an Angle was a good idea. I suspect other variables would benefit from the same treatment, but suggest that be left for a future cleanup pass (maybe one that also removes RecordProxy from the public namespace).

            One remaining request: please update the documentation of matchOptimisticB in matchOptimisticB.h. Presently it says that posRefCat reads "centroid" and "hasCentroid" fields, but that is no longer the case, thanks to your improvements. If you are willing, you could also add that the sourceCat fields read are "Centroid_x", "Centroid_y" and control.sourceFluxField.To aid that I provide the following suggested text, which I verified has a nicer format in HTML (significantly more so than the old text):

                @param[in] posRefCat  catalog of position reference stars; fields read:
                    - "coord"
                    - control.refFluxField
                @param[in] sourceCat  catalog of detected sources; fields read:
                    - "Centroid_x"
                    - "Centroid_y"
                    - control.refFluxField
            

            I will also note a quirk of the Doxygen that I should have noticed before. I have no idea how to fix it and am not proposing you try. I'm just recording it. The documentation for the function "matchOptimisticB" is hard to find. The link of that name under the tab "Namespaces: Namespace List" points to documentation of the python module of that name, not the C++ function. I found documentation for the C++ function under "Files:matchOptimisticB.h". Also also found it under the tab "Namespaces: Namespace Members", but that page is really cluttered and clumsy to use and the linked entry includes an unwanted entry for the function from the .cc file, even though the .cc file contains no doxygen comments.

            Show
            rowen Russell Owen added a comment - Many thanks for the enhancements. Changing theta to an Angle was a good idea. I suspect other variables would benefit from the same treatment, but suggest that be left for a future cleanup pass (maybe one that also removes RecordProxy from the public namespace). One remaining request: please update the documentation of matchOptimisticB in matchOptimisticB.h. Presently it says that posRefCat reads "centroid" and "hasCentroid" fields, but that is no longer the case, thanks to your improvements. If you are willing, you could also add that the sourceCat fields read are "Centroid_x", "Centroid_y" and control.sourceFluxField.To aid that I provide the following suggested text, which I verified has a nicer format in HTML (significantly more so than the old text): @param[in] posRefCat catalog of position reference stars; fields read: - "coord" - control.refFluxField @param[in] sourceCat catalog of detected sources; fields read: - "Centroid_x" - "Centroid_y" - control.refFluxField I will also note a quirk of the Doxygen that I should have noticed before. I have no idea how to fix it and am not proposing you try. I'm just recording it. The documentation for the function "matchOptimisticB" is hard to find. The link of that name under the tab "Namespaces: Namespace List" points to documentation of the python module of that name, not the C++ function. I found documentation for the C++ function under "Files:matchOptimisticB.h". Also also found it under the tab "Namespaces: Namespace Members", but that page is really cluttered and clumsy to use and the linked entry includes an unwanted entry for the function from the .cc file, even though the .cc file contains no doxygen comments.
            Hide
            price Paul Price added a comment -

            Thanks Russell. I updated the doc, rebased and merged to master.

            Show
            price Paul Price added a comment - Thanks Russell. I updated the doc, rebased and merged to master.

              People

              Assignee:
              price Paul Price
              Reporter:
              price Paul Price
              Reviewers:
              Russell Owen
              Watchers:
              Jim Bosch, Paul Price, Russell Owen, Simon Krughoff
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins Builds

                  No builds found.