Details

    • Templates:
    • Story Points:
      4
    • Sprint:
      DRP X16-3, DRP F16-1
    • Team:
      Data Release Production

      Description

      The new colorterms code that we adopted from HSC may not have complete unit tests. The existing colorterms test is pretty good, but may have holes. I'm more concerned about the unit test for PhotoCalTask, which does not apply colorterms at all (likely an existing issue).

      Also, be sure to test that the obs_cfht config override loads correctly (presumably with a unit test in obs_cfht) and similarly for obs_subaru.

        Issue Links

          Activity

          Hide
          vpk24 Vishal Kasliwal [X] (Inactive) added a comment -

          Hi Pim,
          Can you please give this ticket a review?

          Show
          vpk24 Vishal Kasliwal [X] (Inactive) added a comment - Hi Pim, Can you please give this ticket a review?
          Hide
          pschella Pim Schellart added a comment -

          I have reviewed the code (see PR) and apart from some minor comments it looks fine. However I am not sure I can properly judge if the work done really covers the ticket. So I'm assigning John Swinbank to have a second look.

          Show
          pschella Pim Schellart added a comment - I have reviewed the code (see PR) and apart from some minor comments it looks fine. However I am not sure I can properly judge if the work done really covers the ticket. So I'm assigning John Swinbank to have a second look.
          Hide
          swinbank John Swinbank added a comment -

          Added a few minor style comments to the obs_subaru PR. Effectively the same things apply to obs_cfht; I'm not going to bother to re-list them there.

          I'm sceptical of the value of checking the values of numbers in the obs package tests. This just means that the numbers are going to have to be changed twice whenever they need to be updated – what is this buying us?

          Testing that "that the... config override loads correctly" seems a bit vague to me. I think you're basically doing what Russell Owen was looking for, but maybe he can confirm – perhaps he actually wanted you to instantiate a command line task or something?

          I think the tests in pipe_tasks are pretty much what we're looking for. I've not carefully checked the colorterms code (or run a coverage tool) to make sure there are no major code paths being missed, but this certainly looks reasonable.

          However, I do think you should revisit the code in testPhotoCal.py. There's a lot of duplication between the old PhotoCalTest.test1 and PhotoCalTest.testColorTerms. A little bit of refactoring here could clean things up a lot. It would also be helpful to add some comments describing what it is you're testing and why you're getting the results you expect.

          Show
          swinbank John Swinbank added a comment - Added a few minor style comments to the obs_subaru PR. Effectively the same things apply to obs_cfht; I'm not going to bother to re-list them there. I'm sceptical of the value of checking the values of numbers in the obs package tests. This just means that the numbers are going to have to be changed twice whenever they need to be updated – what is this buying us? Testing that "that the... config override loads correctly" seems a bit vague to me. I think you're basically doing what Russell Owen was looking for, but maybe he can confirm – perhaps he actually wanted you to instantiate a command line task or something? I think the tests in pipe_tasks are pretty much what we're looking for. I've not carefully checked the colorterms code (or run a coverage tool) to make sure there are no major code paths being missed, but this certainly looks reasonable. However, I do think you should revisit the code in testPhotoCal.py . There's a lot of duplication between the old PhotoCalTest.test1 and PhotoCalTest.testColorTerms . A little bit of refactoring here could clean things up a lot. It would also be helpful to add some comments describing what it is you're testing and why you're getting the results you expect.
          Hide
          rowen Russell Owen added a comment -

          I agree with John Swinbank that checking the values is a bit worrisome. But having it all together in the obs_ package makes it less of a concern than it would otherwise be. At some point I assume colorterm data will be treated like other calibration data, with timestamps, and moved out of the obs_ packages. At that point the test will probably need to be reworked. So I am ambivalent about the value checking; you could leave it, broaden the tolerance to the point that it is likely to accept updated values, or remove it.

          Show
          rowen Russell Owen added a comment - I agree with John Swinbank that checking the values is a bit worrisome. But having it all together in the obs_ package makes it less of a concern than it would otherwise be. At some point I assume colorterm data will be treated like other calibration data, with timestamps, and moved out of the obs_ packages. At that point the test will probably need to be reworked. So I am ambivalent about the value checking; you could leave it, broaden the tolerance to the point that it is likely to accept updated values, or remove it.
          Hide
          vpk24 Vishal Kasliwal [X] (Inactive) added a comment -

          Merged to master...

          Show
          vpk24 Vishal Kasliwal [X] (Inactive) added a comment - Merged to master...
          Show
          swinbank John Swinbank added a comment - Mentioned in release notes: https://confluence.lsstcorp.org/display/DM/Data+Release+Production+WIP+F16+Release+Notes

            People

            • Assignee:
              vpk24 Vishal Kasliwal [X] (Inactive)
              Reporter:
              rowen Russell Owen
              Reviewers:
              John Swinbank, Pim Schellart
              Watchers:
              Jim Bosch, John Swinbank, Kian-Tat Lim, Pim Schellart, Robert Lupton, Russell Owen, Vishal Kasliwal [X] (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile