Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: obs_cfht, obs_subaru, pipe_tasks
-
Story Points:4
-
Epic Link:
-
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.
Attachments
Issue Links
Activity
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.
Pim Schellart [X] (Inactive)
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. 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.
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.
Merged to master...
Mentioned in release notes: https://confluence.lsstcorp.org/display/DM/Data+Release+Production+WIP+F16+Release+Notes
Hi Pim,
Can you please give this ticket a review?