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

not flagged NaN in sdssCentroid

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base
    • Labels:
      None

      Description

      In some rare cases base_SdssCentroid_x(y)Sigma can be NaN while base_SdssCentroid_flag is False

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            Hi Ian —

            As discussed earlier, here is a small review. PR is here:

            https://github.com/lsst/meas_base/pull/150

            Jenkins is here:

            https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/30595/pipeline/49/

            (Note that the failures on Linux systems seem to be due to Docker Hub problems; I'm optimistic that all is fine given the success on macOS).

            I am reluctant to get into the details of the measurement algorithm on this ticket. It may be that SdssCentroid should never produce NaN errors (although the code seems to expect it...), but I think that's out of scope here: all we're interested in is are those NaNs being properly flagged when they occur. (If you'd like, we can make a ticket to follow up on the algorithmic details later.)

            Unfortunately, the algorithmic code is rather involved. That makes it very hard to produce a good test case for this — I can't easily generate synthetic data which will cause the algorithm to succeed in all ways except that it generates NaNs for xErr and/or yErr. Given time constraints, and that the changes here are modest, I'd prefer to get this merged as-is rather than spend a long time examining the algorithm and agonizing over how to test it. If you think that attitude is unsustainable, let's discuss. (Even better, if you can suggest a way to test this without pulling the algorithm apart, please do let me know!!!)

            Show
            swinbank John Swinbank added a comment - Hi Ian — As discussed earlier, here is a small review. PR is here: https://github.com/lsst/meas_base/pull/150 Jenkins is here: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/30595/pipeline/49/ (Note that the failures on Linux systems seem to be due to Docker Hub problems; I'm optimistic that all is fine given the success on macOS). I am reluctant to get into the details of the measurement algorithm on this ticket. It may be that SdssCentroid should never produce NaN errors (although the code seems to expect it ...), but I think that's out of scope here: all we're interested in is are those NaNs being properly flagged when they occur. (If you'd like, we can make a ticket to follow up on the algorithmic details later.) Unfortunately, the algorithmic code is rather involved. That makes it very hard to produce a good test case for this — I can't easily generate synthetic data which will cause the algorithm to succeed in all ways except that it generates NaNs for xErr and/or yErr . Given time constraints, and that the changes here are modest, I'd prefer to get this merged as-is rather than spend a long time examining the algorithm and agonizing over how to test it. If you think that attitude is unsustainable, let's discuss. (Even better, if you can suggest a way to test this without pulling the algorithm apart, please do let me know!!!)
            Hide
            sullivan Ian Sullivan added a comment -

            I may have clicked the wrong status button at first, but I think I have set it correctly now. The changes look fine, though I had one alternate suggestion on the PR to consider.

            Show
            sullivan Ian Sullivan added a comment - I may have clicked the wrong status button at first, but I think I have set it correctly now. The changes look fine, though I had one alternate suggestion on the PR to consider.
            Hide
            sullivan Ian Sullivan added a comment -

            The new changes look good. I had a few minor comments on docstrings.

            Show
            sullivan Ian Sullivan added a comment - The new changes look good. I had a few minor comments on docstrings.
            Hide
            swinbank John Swinbank added a comment -

            Thanks! Merged & done.

            Show
            swinbank John Swinbank added a comment - Thanks! Merged & done.

              People

              • Assignee:
                swinbank John Swinbank
                Reporter:
                boutigny Dominique Boutigny
                Reviewers:
                Ian Sullivan
                Watchers:
                Dominique Boutigny, Ian Sullivan, John Swinbank
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel