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

PhotoCalTask mis-calling Colorterm methods

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: pipe_tasks
    • Labels:
      None

      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

            Hide
            krughoff Simon Krughoff added a comment -

            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.

            Show
            krughoff Simon Krughoff added a comment - 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.
            Hide
            rowen Russell Owen added a comment -

            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.

            Show
            rowen Russell Owen added a comment - 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.
            Hide
            krughoff Simon Krughoff added a comment -

            Looks fine now. Thanks.

            Show
            krughoff Simon Krughoff added a comment - Looks fine now. Thanks.

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Simon Krughoff
              Watchers:
              Lauren MacArthur, Russell Owen, Simon Krughoff
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: