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

meas_astrom bugs exposed by new Eigen

    Details

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

      Description

      Trying a newer Eigen has exposed several issues in meas_astrom:

      • tests/createWcsWithSip.py blindly uses sipWcs in the result returned by ANetBasicAstrometryTask.determineWcs2, but that attribute may be None
      • ANetBasicAstrometryTask.determineWcs2 terminates iteration early if the # of matches goes down, even though the result may be improved. In the case in question the first fit iteration results in significantly better RMS error, but has one fewer matches, so the SIP fit is rejected, triggering the first bug mentioned.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            I fixed the issue. It was a stupid bug that I had introduced in my final commit.

            Show
            rowen Russell Owen added a comment - I fixed the issue. It was a stupid bug that I had introduced in my final commit.
            Hide
            rowen Russell Owen added a comment -

            Tim Jenness made some helpful suggestions on the pull request. I responded by merging and rebasing a few changes.

            Show
            rowen Russell Owen added a comment - Tim Jenness made some helpful suggestions on the pull request. I responded by merging and rebasing a few changes.
            Hide
            krughoff Simon Krughoff added a comment -

            This all looks fine. I have minor comments on the PR, mostly related to code duplication.

            Merge at your leisure.

            Show
            krughoff Simon Krughoff added a comment - This all looks fine. I have minor comments on the PR, mostly related to code duplication. Merge at your leisure.
            Hide
            rowen Russell Owen added a comment - - edited

            I updated an incorrect doc string and pushed back on the other suggested changes, ungrateful wretch that I am.

            For the record the main area of disagreement appears to be whether there should be variants of makeMatchStatisticsInRadians/InPixels that don't take a WCS. I argue that these assume too much about the match list (especially while fitting the WCS, during which the necessary information may be unavailable, or, heaven forbid, stale), and that the existing non-WCS variant that computes statistics based on the distance field of the match is preferred once the match list is fully updated. However, Simon Krughoff has a point; it can be useful to get distance on-sky or in pixels (instead of being stuck with whatever is in the distance field) and the provided functions for that are less efficient than they could be because they rely on a WCS. At this point I relegate such additional functions to a future ticket.

            Another issue I have minor concerns about is that each the loops that refine the WCS solution work differently in AstrometryTask (AT) and ANetBasicAstrometryTask (ANBAT):

            • ANBAT takes a pure tangent WCS and a match list, and for each iteration it first fits a WCS and then performs a new match based on that WCS.
            • AT does the opposite: it doesn't even take a match list, but for each loop it first finds one, then fits a WCS to it.
              I spent a bit of time trying the AT approach in ANBAT but it did not work quite as well (perhaps because the initial WCS is pure tangent, ignoring optical distortion). The AT approach seems to work fine so I didn't want to change that task. I concluded it was safer to leave both tasks working as they were.
            Show
            rowen Russell Owen added a comment - - edited I updated an incorrect doc string and pushed back on the other suggested changes, ungrateful wretch that I am. For the record the main area of disagreement appears to be whether there should be variants of makeMatchStatisticsInRadians/InPixels that don't take a WCS. I argue that these assume too much about the match list (especially while fitting the WCS, during which the necessary information may be unavailable, or, heaven forbid, stale), and that the existing non-WCS variant that computes statistics based on the distance field of the match is preferred once the match list is fully updated. However, Simon Krughoff has a point; it can be useful to get distance on-sky or in pixels (instead of being stuck with whatever is in the distance field) and the provided functions for that are less efficient than they could be because they rely on a WCS. At this point I relegate such additional functions to a future ticket. Another issue I have minor concerns about is that each the loops that refine the WCS solution work differently in AstrometryTask (AT) and ANetBasicAstrometryTask (ANBAT): ANBAT takes a pure tangent WCS and a match list, and for each iteration it first fits a WCS and then performs a new match based on that WCS. AT does the opposite: it doesn't even take a match list, but for each loop it first finds one, then fits a WCS to it. I spent a bit of time trying the AT approach in ANBAT but it did not work quite as well (perhaps because the initial WCS is pure tangent, ignoring optical distortion). The AT approach seems to work fine so I didn't want to change that task. I concluded it was safer to leave both tasks working as they were.
            Hide
            rowen Russell Owen added a comment -

            Merged to master and pushed

            Show
            rowen Russell Owen added a comment - Merged to master and pushed

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Simon Krughoff
                Watchers:
                Jim Bosch, Russell Owen, Simon Krughoff, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel