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

Raise when there are not enough matched stars in astrometric fitting

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: To Do
    • Resolution: Unresolved
    • Fix Version/s: None
    • Component/s: meas_astrom
    • Labels:
      None
    • Team:
      Data Release Production
    • Urgent?:
      No

      Description

      There are occasional silent failures of the astrometry task in processCcd, that Dominique Boutigny has long complained about on Slack: e.g., https://lsstc.slack.com/archives/C978LTJGN/p1526484056000588 but that never got ticketed.

      The gist of the problem is that sometimes the astrometry fails badly (in particular in many DESC DC2 u-band ccds, but there are probably other examples) but the fitter thinks everything is fine.  This has led to Dominique Boutigny's script to check for these failures:  https://github.com/LSSTDESC/ImageProcessingPipelines/blob/master/python/util/checkCcdAstrometry.py (see DM-16301 for a ticket to add the logic from this script to processCcd).

      Recent investigation (see, e.g., https://lsstc.slack.com/archives/C978LTJGN/p1543335472430300 and following) makes it seem like the culprit for most (all?) of these silent failures is that the fitter is trying to fit more SIP terms than there are constraints from the stars.  When these stars are in only one section of the CCD, this leads to overfitting for the matched stars and wild extrapolations for other stars, leading to internal statistics from the fitter looking "good" but the overall fit being bad.

      (In general, if the default order of the SIP fit is reduced to 2 this seems to greatly increase the success rate on simulated u-band images).

      Specifically, for this ticket, I propose having the fitter raise an exception of there are not enough matched stars for the SIP order that was requested.  This would turn silent failures into loud failures, as well as alerting the user of where things were going wrong.

        Attachments

          Issue Links

            Activity

            No builds found.
            erykoff Eli Rykoff created issue -
            rhl Robert Lupton made changes -
            Field Original Value New Value
            Watchers Dominique Boutigny, Eli Rykoff, John Parejko [ Dominique Boutigny, Eli Rykoff, John Parejko ] Dominique Boutigny, Eli Rykoff, John Parejko, John Swinbank [ Dominique Boutigny, Eli Rykoff, John Parejko, John Swinbank ]
            swinbank John Swinbank made changes -
            Description There are occasional silent failures of the astrometry task in processCcd, that [~boutigny] has long complained about on Slack: e.g., [https://lsstc.slack.com/archives/C978LTJGN/p1526484056000588] but that never got ticketed.

            The gist of the problem is that sometimes the astrometry fails badly (in particular in many DESC DC2 u-band ccds, but there are probably other examples) but the fitter things everything is fine.  This has led to [~boutigny]'s script to check for these failures:  [https://github.com/LSSTDESC/ImageProcessingPipelines/blob/master/python/util/checkCcdAstrometry.py] (see DM-16301 for a ticket to add the logic from this script to processCcd).

            Recent investigation (see, e.g., [https://lsstc.slack.com/archives/C978LTJGN/p1543335472430300] and following) makes it seem like the culprit for most (all?) of these silent failures is that the fitter is trying to fit more SIP terms than there are constraints from the stars.  When these stars are in only one section of the CCD, this leads to overfitting for the matched stars and wild extrapolations for other stars, leading to internal statistics from the fitter looking "good" but the overall fit being bad.

            (In general, if the default order of the SIP fit is reduced to 2 this seems to greatly increase the success rate on simulated u-band images).

            Specifically, for this ticket, I propose having the fitter raise an exception of there are not enough matched stars for the SIP order that was requested.  This would turn silent failures into loud failures, as well as alerting the user of where things were going wrong.
            There are occasional silent failures of the astrometry task in processCcd, that [~boutigny] has long complained about on Slack: e.g., [https://lsstc.slack.com/archives/C978LTJGN/p1526484056000588] but that never got ticketed.

            The gist of the problem is that sometimes the astrometry fails badly (in particular in many DESC DC2 u-band ccds, but there are probably other examples) but the fitter thinks everything is fine.  This has led to [~boutigny]'s script to check for these failures:  [https://github.com/LSSTDESC/ImageProcessingPipelines/blob/master/python/util/checkCcdAstrometry.py] (see DM-16301 for a ticket to add the logic from this script to processCcd).

            Recent investigation (see, e.g., [https://lsstc.slack.com/archives/C978LTJGN/p1543335472430300] and following) makes it seem like the culprit for most (all?) of these silent failures is that the fitter is trying to fit more SIP terms than there are constraints from the stars.  When these stars are in only one section of the CCD, this leads to overfitting for the matched stars and wild extrapolations for other stars, leading to internal statistics from the fitter looking "good" but the overall fit being bad.

            (In general, if the default order of the SIP fit is reduced to 2 this seems to greatly increase the success rate on simulated u-band images).

            Specifically, for this ticket, I propose having the fitter raise an exception of there are not enough matched stars for the SIP order that was requested.  This would turn silent failures into loud failures, as well as alerting the user of where things were going wrong.
            swinbank John Swinbank made changes -
            Summary There are occasional silent failures of the astrometry task in processCcd Raise when there are not enough matched stars in astrometric fitting
            swinbank John Swinbank made changes -
            Priority Undefined [ 10000 ] Major [ 3 ]
            swinbank John Swinbank made changes -
            Component/s meas_astrom [ 10745 ]
            Component/s Data Release Production [ 13901 ]
            Hide
            swinbank John Swinbank added a comment -

            Yusra AlSayyad — it'd be nice to get to this one, and DM-16301, soon. I think you told me the latter was going to be handled by the HSC team — is that right? Any chance of folding this into it?

            Otherwise, I can put this on the backlog for AP and try to prioritise it.

            Show
            swinbank John Swinbank added a comment - Yusra AlSayyad — it'd be nice to get to this one, and DM-16301 , soon. I think you told me the latter was going to be handled by the HSC team — is that right? Any chance of folding this into it? Otherwise, I can put this on the backlog for AP and try to prioritise it.
            swinbank John Swinbank made changes -
            Team Data Release Production [ 10301 ]
            Hide
            erykoff Eli Rykoff added a comment -

            John Swinbank While I appreciate the updates to the ticket title, which is more accurate for the actionable part of the ticket, I wanted to make sure that this fix will not necessarily fix all the silent failures.  I believe we can just file a new ticket if we find more silent failures after this fix is put in, but we want to make sure that ticket link to this one should it be necessary.

            At the same time, is it appropriate on this ticket to change the default order for the astrometry fitting in processCcd?  

            Show
            erykoff Eli Rykoff added a comment - John Swinbank While I appreciate the updates to the ticket title, which is more accurate for the actionable part of the ticket, I wanted to make sure that this fix will not necessarily fix all the silent failures.  I believe we can just file a new ticket if we find more silent failures after this fix is put in, but we want to make sure that ticket link to this one should it be necessary. At the same time, is it appropriate on this ticket to change the default order for the astrometry fitting in processCcd?  
            Hide
            swinbank John Swinbank added a comment -

            John Swinbank While I appreciate the updates to the ticket title, which is more accurate for the actionable part of the ticket, I wanted to make sure that this fix will not necessarily fix all the silent failures. I believe we can just file a new ticket if we find more silent failures after this fix is put in, but we want to make sure that ticket link to this one should it be necessary

            Understood. As you indicate, the aim here is to acknowledge that tickets are actionable. If you like, we could file another ticket now to audit the codebase and check that there's no possibility of an silent failure, or we could ticket them on a case-by-case basis as we come to them.

            At the same time, is it appropriate on this ticket to change the default order for the astrometry fitting in processCcd?

            .

            Show
            swinbank John Swinbank added a comment - John Swinbank While I appreciate the updates to the ticket title, which is more accurate for the actionable part of the ticket, I wanted to make sure that this fix will not necessarily fix all the silent failures. I believe we can just file a new ticket if we find more silent failures after this fix is put in, but we want to make sure that ticket link to this one should it be necessary Understood. As you indicate, the aim here is to acknowledge that tickets are actionable. If you like, we could file another ticket now to audit the codebase and check that there's no possibility of an silent failure, or we could ticket them on a case-by-case basis as we come to them. At the same time, is it appropriate on this ticket to change the default order for the astrometry fitting in processCcd? .
            Hide
            yusra Yusra AlSayyad added a comment -

            John Swinbank, We could definitely fold this into DM-16301, but we promised HSC that LSST would do this, not the other way around.

            Everyone vote for this and DM-16301 if you want DRP to do this as part of pair programming this week.  Otherwise, it's scheduled for the Astrometry QA sprint in Feb.

             

            Show
            yusra Yusra AlSayyad added a comment - John Swinbank , We could definitely fold this into DM-16301 , but we promised HSC that LSST would do this, not the other way around. Everyone vote for this and DM-16301 if you want DRP to do this as part of pair programming this week.  Otherwise, it's scheduled for the Astrometry QA sprint in Feb.  
            lskelvin Lee Kelvin made changes -
            Assignee Lee Kelvin [ lskelvin ]
            Hide
            lskelvin Lee Kelvin added a comment - - edited

            @Eli Rykoff: Dan Taranu, Morgan Schmitz and I have been looking into this ticket as part of a pair coding exercise. We've added additional loggers that should ensure that this issue no longer leads to a silent failure. Specifically, if the number of matched sources (numMatches) is less than 10 we raise a TaskError. We also log the number of rejected matches, for reference. Our only outstanding question is how to choose an accurate value for numMatches? The original ticket recommends that the minimum number of matches should vary as a function of the SIP order. We'd like to know if 10 is an acceptable value for us to use here, or if there is a better way to generate this number.

            FYI, we've been testing this by running this command:

            $ processCcd.py /datasets/DC2/repoRun2.2i --output ../test --id expId=466758 --clobber-config --no-versions

            Show
            lskelvin Lee Kelvin added a comment - - edited @ Eli Rykoff : Dan Taranu , Morgan Schmitz and I have been looking into this ticket as part of a pair coding exercise. We've added additional loggers that should ensure that this issue no longer leads to a silent failure. Specifically, if the number of matched sources (numMatches) is less than 10 we raise a TaskError . We also log the number of rejected matches, for reference. Our only outstanding question is how to choose an accurate value for numMatches? The original ticket recommends that the minimum number of matches should vary as a function of the SIP order. We'd like to know if 10 is an acceptable value for us to use here, or if there is a better way to generate this number. FYI, we've been testing this by running this command: $ processCcd.py /datasets/DC2/repoRun2.2i --output ../test --id expId=466758 --clobber-config --no-versions
            Parejkoj John Parejko made changes -
            Watchers Dominique Boutigny, Eli Rykoff, John Parejko, John Swinbank, Lee Kelvin, Yusra AlSayyad [ Dominique Boutigny, Eli Rykoff, John Parejko, John Swinbank, Lee Kelvin, Yusra AlSayyad ] Chris Morrison, Dominique Boutigny, Eli Rykoff, John Parejko, John Swinbank, Lee Kelvin, Yusra AlSayyad [ Chris Morrison, Dominique Boutigny, Eli Rykoff, John Parejko, John Swinbank, Lee Kelvin, Yusra AlSayyad ]
            Hide
            Parejkoj John Parejko added a comment -

            Are your changes specific to the SIP fitter, or generic to all fitters? The Affine fitter should do ok on fewer sources than the SIP fitter.

            Whatever that number is, it should be made configurable.

            Show
            Parejkoj John Parejko added a comment - Are your changes specific to the SIP fitter, or generic to all fitters? The Affine fitter should do ok on fewer sources than the SIP fitter. Whatever that number is, it should be made configurable.
            Hide
            dtaranu Dan Taranu added a comment -

            We only added it to FitTanSipWcsTask, but we could to the same for the affine fitter too. Adding a config parameter seems reasonable. Like Lee, I skimmed through the original discussion on slack about the required number of free parameters for a given order and didn't see a conclusive answer. So, I'm not sure what to set the default to or whether it needs to be validated (i.e. raise or warn if the user sets the minimum matches to zero, or less than say ten stars for the default 2nd order).

            Show
            dtaranu Dan Taranu added a comment - We only added it to FitTanSipWcsTask, but we could to the same for the affine fitter too. Adding a config parameter seems reasonable. Like Lee, I skimmed through the original discussion on slack about the required number of free parameters for a given order and didn't see a conclusive answer. So, I'm not sure what to set the default to or whether it needs to be validated (i.e. raise or warn if the user sets the minimum matches to zero, or less than say ten stars for the default 2nd order).
            Hide
            dtaranu Dan Taranu added a comment -

            On a related note: Dominique Boutigny if you have examples of problematic fits that we can reproduce on lsst-dev, that would be very helpful.

            Show
            dtaranu Dan Taranu added a comment - On a related note: Dominique Boutigny if you have examples of problematic fits that we can reproduce on lsst-dev, that would be very helpful.
            Hide
            lskelvin Lee Kelvin added a comment -

            On closer inspection, it appears that CreateWcsWithSip.cc (line 130) already implements SIP order checking against matched star numbers (post-rejection), and so the rationale for this ticket may have been removed. Specifically, it checks that the number of matched stars is greater than order+1, which we believe to be the correct logic in this case.

            To wrap up this ticket, I propose that we leave in the python-side info logger code we've added which provides the number of rejected stars if greater than zero, as this information wasn't immediately obvious beforehand.

            Show
            lskelvin Lee Kelvin added a comment - On closer inspection, it appears that CreateWcsWithSip.cc (line 130) already implements SIP order checking against matched star numbers (post-rejection), and so the rationale for this ticket may have been removed. Specifically, it checks that the number of matched stars is greater than order+1, which we believe to be the correct logic in this case. To wrap up this ticket, I propose that we leave in the python-side info logger code we've added which provides the number of rejected stars if greater than zero, as this information wasn't immediately obvious beforehand.
            Hide
            erykoff Eli Rykoff added a comment -

            My guess is that that cut is the bare minimum for the code not to crash, but in practice is insufficient to ensure the fit actually makes sense.

            Show
            erykoff Eli Rykoff added a comment - My guess is that that cut is the bare minimum for the code not to crash, but in practice is insufficient to ensure the fit actually makes sense.
            Hide
            lskelvin Lee Kelvin added a comment -

            I agree - the trouble is that this ticket overlaps with DM-16301, which seemingly better addresses that issue. For the purposes of this ticket (having the fitter raise an exception if there are not enough matched stars for the SIP order that was requested), it looks like this is already being done within CreateWcsWithSip.cc, so we're not sure what else can/should be added here.

            Show
            lskelvin Lee Kelvin added a comment - I agree - the trouble is that this ticket overlaps with DM-16301 , which seemingly better addresses that issue. For the purposes of this ticket (having the fitter raise an exception if there are not enough matched stars for the SIP order that was requested), it looks like this is already being done within CreateWcsWithSip.cc , so we're not sure what else can/should be added here.
            lskelvin Lee Kelvin made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            lskelvin Lee Kelvin made changes -
            Link This issue relates to DM-17737 [ DM-17737 ]
            lskelvin Lee Kelvin made changes -
            Link This issue relates to DM-16301 [ DM-16301 ]
            lskelvin Lee Kelvin made changes -
            Urgent? off
            Hide
            lskelvin Lee Kelvin added a comment -

            Following the discussion on DM-16301, I'm also pausing this ticket until further 'successfully failing' data IDs are available for testing.

            Show
            lskelvin Lee Kelvin added a comment - Following the discussion on DM-16301 , I'm also pausing this ticket until further 'successfully failing' data IDs are available for testing.
            lskelvin Lee Kelvin made changes -
            Status In Progress [ 3 ] To Do [ 10001 ]
            lskelvin Lee Kelvin made changes -
            Assignee Lee Kelvin [ lskelvin ]

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              erykoff Eli Rykoff
              Watchers:
              Chris Morrison [X] (Inactive), Dan Taranu, Dominique Boutigny, Eli Rykoff, John Parejko, Lee Kelvin, Yusra AlSayyad
              Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:

                  Jenkins

                  No builds found.