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

Implement HSC improvements to Colorterm

    XMLWordPrintable

    Details

      Description

      Paul Price recommended some HSC changes for the Colorterm class. To quote Paul: changed Colorterms so it's not a global, and it can now be configured using Config. See https://github.com/HyperSuprime-Cam/meas_astrom/blob/master/python/lsst/meas/photocal/colorterms.py

      This sounds useful. Note that the HSC colorterms.py is in meas_astrom but as of DM-1578 the LSST version is in pipe_tasks.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            One problem with this change: the new PhotoCalTask will fail by default if no colorterms data is provided. That's why tests/testPhotoCalTask.py had to be modified to set config.applyColorTerms to False, and it also appears to be the reason why the current integration test is failing. The solutions I've thought of are:

            • Set the default value of config.applyColorTerms in obs_* packages to True if colorterms data is available in that package, False otherwise. Since the colorterm data is available in the obs_* packages that makes a lot of sense, though I could wish that the flag was set in the colorterms config override file itself, so as to not split the related info into two separate override files. Similarly it would be nice to set the reference catalog name in this same file.
            • In the PhotoCalTask's config set the task default value of config.applyColorTerms to True if colorterms data is available, and False otherwise. That feels a bit too magic, but if we can trust our loading of colorterms data it ought to work.
            • Set the task default for config.applyColorTerms to False. This avoids exceptions at the cost of possibly surprising users by not applying color terms.
            • Live with it and update our integration task. I'm uncomfortable with this because it will bite lots of users.
            Show
            rowen Russell Owen added a comment - One problem with this change: the new PhotoCalTask will fail by default if no colorterms data is provided. That's why tests/testPhotoCalTask.py had to be modified to set config.applyColorTerms to False, and it also appears to be the reason why the current integration test is failing. The solutions I've thought of are: Set the default value of config.applyColorTerms in obs_* packages to True if colorterms data is available in that package, False otherwise. Since the colorterm data is available in the obs_* packages that makes a lot of sense, though I could wish that the flag was set in the colorterms config override file itself, so as to not split the related info into two separate override files. Similarly it would be nice to set the reference catalog name in this same file. In the PhotoCalTask's config set the task default value of config.applyColorTerms to True if colorterms data is available, and False otherwise. That feels a bit too magic, but if we can trust our loading of colorterms data it ought to work. Set the task default for config.applyColorTerms to False. This avoids exceptions at the cost of possibly surprising users by not applying color terms. Live with it and update our integration task. I'm uncomfortable with this because it will bite lots of users.
            Hide
            rhl Robert Lupton added a comment -

            I care more about the user experience than the code aesthetics, so let's go with one of your first two options; I prefer the second option as I'd worry about people adding colour terms, but not setting applyColorTerms to True

            Show
            rhl Robert Lupton added a comment - I care more about the user experience than the code aesthetics, so let's go with one of your first two options; I prefer the second option as I'd worry about people adding colour terms, but not setting applyColorTerms to True
            Hide
            rowen Russell Owen added a comment - - edited

            Robert: on the whole I agree with you. However, I will point out one advantage to using the first option: one can also set the default reference catalog to a better value than "*" (an unusable value if there is color terms data for more than one reference catalog). Setting the default catalog and the value of the applyColorTerms would be done in the same file – though unfortunately, not the file that holds the colorterms data.

            This whole business of choosing a default reference catalog worries me. I think HSC had a better solution, because it was based on whatever reference catalog was setup. But that doesn't translate well to moving away from astrometry_net.

            Show
            rowen Russell Owen added a comment - - edited Robert: on the whole I agree with you. However, I will point out one advantage to using the first option: one can also set the default reference catalog to a better value than "*" (an unusable value if there is color terms data for more than one reference catalog). Setting the default catalog and the value of the applyColorTerms would be done in the same file – though unfortunately, not the file that holds the colorterms data. This whole business of choosing a default reference catalog worries me. I think HSC had a better solution, because it was based on whatever reference catalog was setup. But that doesn't translate well to moving away from astrometry_net.
            Hide
            jbosch Jim Bosch added a comment -

            A few minor comments on the pipe_tasks GitHub PR.

            I've essentially ignored the larger design concerns here, as I think they're already acknowledged to be beyond the scope of this particular issue.

            Show
            jbosch Jim Bosch added a comment - A few minor comments on the pipe_tasks GitHub PR. I've essentially ignored the larger design concerns here, as I think they're already acknowledged to be beyond the scope of this particular issue.
            Hide
            rowen Russell Owen added a comment - - edited

            I made the following changes since the review:

            • PhotoCalConfig.applyColorTerms now is tri-state. True and False retain their old meaning, and in particular True will try to apply the terms and fail if no data is available. The new value None applies color terms if any colorterm data is available, else not. A new log message shows whether the task decided to display colorterm data, and why.
            • I renamed ColortermDictConfig to ColortermDict and ColortermLibraryConfig to ColortermLibrary. I felt these classes (at least the latter) are likely to stick around even after we change how we persist colorterm data, and the fact that they happen to be configs at the moment is irrelevant, except that it complicates the constructors a bit.

            I tried to improve the constructors once more but I cannot make them as clean as I wanted due to DM-2831 so I'll leave out constructors.

            This version passes the integration test.

            Jim: do you want to have another look (or ask me to have somebody else take another look) before I squash and merge, or shall I proceed?

            Show
            rowen Russell Owen added a comment - - edited I made the following changes since the review: PhotoCalConfig.applyColorTerms now is tri-state. True and False retain their old meaning, and in particular True will try to apply the terms and fail if no data is available. The new value None applies color terms if any colorterm data is available, else not. A new log message shows whether the task decided to display colorterm data, and why. I renamed ColortermDictConfig to ColortermDict and ColortermLibraryConfig to ColortermLibrary. I felt these classes (at least the latter) are likely to stick around even after we change how we persist colorterm data, and the fact that they happen to be configs at the moment is irrelevant, except that it complicates the constructors a bit. I tried to improve the constructors once more but I cannot make them as clean as I wanted due to DM-2831 so I'll leave out constructors. This version passes the integration test. Jim: do you want to have another look (or ask me to have somebody else take another look) before I squash and merge, or shall I proceed?

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Jim Bosch
              Watchers:
              Jim Bosch, Paul Price, Robert Lupton, Russell Owen, Simon Krughoff
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.