Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: pipe_tasks
-
Labels:None
-
Story Points:1
-
Epic Link:
-
Sprint:Science Pipelines DM-S15-4
-
Team:Alert Production
Description
When I implemented DM-2797 I made a few errors in pipe_tasks:
- PhotoCalTask mis-calls two methods of Colorterm by providing filterName, which is not needed
- ColortermLibrary.getColorterm mis-handles glob expressions (the two arguments to fnmatch.fnmatch are swapped).
We also need a unit test for applying colorterms, but that will require enough work that I have made a separate ticket for it: DM-2918. Meanwhile I have tested my changes by running Dominique's CFHT demo. This proves that the colorterm code runs, but does not prove that the terms are correctly applied.
Attachments
Issue Links
Activity
Simon was correct that the change to fnmatch was a mistake. I messed up the default for PhotoCal.config.photoCatName ("*" is not legal because that name cannot be a glob; the globs are the names in the ColortermLibrary). I improved the documentation to avoid that mistake in the future, changed the default value to None (newly permitted), changed the behavior of applyColorterms=None to treat photoCatName=None as "don't apply colorterms". I also added a unit test for ColortermsLibrary. The actual applying of colorterms is still not tested. I verified that it runs using Dominique's CFHT demo, but I think testing that PhotoCalTask applies color terms correctly will require enough additional work to justify a different ticket.
Looks fine now. Thanks.
Echoing what I said in the pull request, I'm not sure the change to fnmatch is needed. I would also like to see the unit test as part of this ticket, but if it's too much work I can be convinced to do it elsewhere.