# Correct for distortion in matchOptimisticB astrometry matcher

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
5
• Sprint:
Science Pipelines DM-S15-6
• Team:
Data Release Production

#### 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.

#### Activity

No builds found.
Paul Price created issue -
Hide
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  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  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  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
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(+)
Field Original Value New Value
Reviewers Simon Krughoff [ krughoff ]
Status To Do [ 10001 ] In Review [ 10004 ]
Hide
Simon Krughoff added a comment -

No problem. I'll do it today.

Show
Simon Krughoff added a comment - No problem. I'll do it today.
 Link This issue is duplicated by DM-2793 [ DM-2793 ]
Hide
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
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
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Hide
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
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
Paul Price added a comment -

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

Show
Paul Price added a comment - Simon Krughoff found some more problems, so taking this back until it's working properly.
 Status Reviewed [ 10101 ] In Progress [ 3 ]
Hide
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  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  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  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  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  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  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  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 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(-)
 Story Points 3 5
 Epic Link DM-2025 [ 16156 ]
 Sprint Science Pipelines DM-S15-6 [ 165 ]
Hide
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
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
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
 Link This issue relates to DM-3549 [ DM-3549 ]
 Link This issue relates to DM-2775 [ DM-2775 ]
Hide
Paul Price added a comment -

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

Show
Paul Price added a comment - I propose to take Russell Owen 's comments as the review. Is this OK with you, Simon Krughoff ?
Hide
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
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
Paul Price added a comment -

I've pushed two fixup commits for review:

 commit 6f741decada4b41983ba25d1ca17a12c47231897 Author: Paul Price  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  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
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(-)
 Reviewers Simon Krughoff [ krughoff ] Russell Owen [ rowen ] Status In Progress [ 3 ] In Review [ 10004 ]
Hide
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
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Hide
Paul Price added a comment -

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

Show
Paul Price added a comment - Thanks Russell. I updated the doc, rebased and merged to master.
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]

#### People

Assignee:
Paul Price
Reporter:
Paul Price
Reviewers:
Russell Owen
Watchers:
Jim Bosch, Paul Price, Russell Owen, Simon Krughoff