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

Tighten testProcessCcd thresholds once background model is fixed

    Details

      Description

      Once the meas_algorithms background model is being generated correctly, the original pipe_tasks test that first pointed to this problem in needs to be put back to higher precision. A comment in testProcessCcd.py indicates the assertions that should have their precisions lowered back to pre-py3-porting values. Presently, this test fails with lower thresholds because the background model is different than expected.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            Since the test tolerances were changed, the expected values were updated. From a first look, it's not obvious that the current results, either in Python 2 or Python 3, are within the old tolerances of the currently expected values. I'll have to do some more digging, but it looks like this isn't just a simple case of restoring the old places values.

            Show
            swinbank John Swinbank added a comment - Since the test tolerances were changed, the expected values were updated. From a first look, it's not obvious that the current results, either in Python 2 or Python 3, are within the old tolerances of the currently expected values. I'll have to do some more digging, but it looks like this isn't just a simple case of restoring the old places values.
            Hide
            swinbank John Swinbank added a comment -

            I was wrong about the above: just resetting places seems to do the trick.

            PR: https://github.com/lsst/pipe_tasks/pull/96

            Waiting for DM-8030 to merge before I send this for review, and also planning to check this works on a Mac (which means rebuilding the stack locally).

            Show
            swinbank John Swinbank added a comment - I was wrong about the above: just resetting places seems to do the trick. PR: https://github.com/lsst/pipe_tasks/pull/96 Waiting for DM-8030 to merge before I send this for review, and also planning to check this works on a Mac (which means rebuilding the stack locally).
            Show
            swinbank John Swinbank added a comment - Jenkins: lsst_py3: https://ci.lsst.codes/job/lsst_py3/420/ lsst_distrib: https://ci.lsst.codes/job/lsst_distrib/625/
            Hide
            swinbank John Swinbank added a comment -

            Building on a Mac to test there ran into DM-9316, hence the delay.

            Show
            swinbank John Swinbank added a comment - Building on a Mac to test there ran into DM-9316 , hence the delay.
            Hide
            swinbank John Swinbank added a comment -

            Finally managed to demonstrate that these numbers are correct on a Mac, too.

            Fred Moolekamp, I think you made the original changes here during the Python 3 port. Are you willing to review this work? Should be quick. PR here.

            Show
            swinbank John Swinbank added a comment - Finally managed to demonstrate that these numbers are correct on a Mac, too. Fred Moolekamp , I think you made the original changes here during the Python 3 port. Are you willing to review this work? Should be quick. PR here .
            Hide
            fred3m Fred Moolekamp added a comment -

            Sure, I should be able to have a look at it tonight or tomorrow morning.

            Show
            fred3m Fred Moolekamp added a comment - Sure, I should be able to have a look at it tonight or tomorrow morning.
            Hide
            fred3m Fred Moolekamp added a comment -

            This looks good, and I see that you reverted to the previous tolerance of 1e-7. That's fine, although at some point it would be nice to have justifications in our tests as to why particular tolerances were chosen. But of course that would be outside the scope of this ticket.

            Show
            fred3m Fred Moolekamp added a comment - This looks good, and I see that you reverted to the previous tolerance of 1e-7. That's fine, although at some point it would be nice to have justifications in our tests as to why particular tolerances were chosen. But of course that would be outside the scope of this ticket.
            Hide
            swinbank John Swinbank added a comment -

            Thanks Fred; merged.

            I broadly agree with your comment about tolerances, although obviously justifying them in detail is pretty hard.

            Show
            swinbank John Swinbank added a comment - Thanks Fred; merged. I broadly agree with your comment about tolerances, although obviously justifying them in detail is pretty hard.

              People

              • Assignee:
                swinbank John Swinbank
                Reporter:
                mrawls Meredith Rawls
                Reviewers:
                Fred Moolekamp
                Watchers:
                Fred Moolekamp, John Swinbank, Meredith Rawls, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: