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

Restore star selector registry

    XMLWordPrintable

    Details

      Description

      Restore the registry for star selectors that was lost in DM-5532, now that tasks in registries can be used as subtasks.

      Also use the registry where appropriate.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment - - edited

            I updated MeasurePsfTask and MeasureApCorrTask to specify the star selector using the registry. This required minor changes to a few config override files and unit tests.

            This consists of small changes to a number of packages. Some packages contain star selectors that need to register themselves, and some packages contain config overrides that needed updating.

            One possibly controversial change I made was to rename the base class for star selectors from StarSelectorTask to BaseStarSelectorTask, in order to clarify that it is an abstract base class.This change is fairly innocuous in that it only affects star selectors (since they inherit from the base class), not users of those star selectors. I used a similar naming convention in DM-6077 making PSF determiners into tasks, which is what prompted the change.

            Show
            rowen Russell Owen added a comment - - edited I updated MeasurePsfTask and MeasureApCorrTask to specify the star selector using the registry. This required minor changes to a few config override files and unit tests. This consists of small changes to a number of packages. Some packages contain star selectors that need to register themselves, and some packages contain config overrides that needed updating. One possibly controversial change I made was to rename the base class for star selectors from StarSelectorTask to BaseStarSelectorTask , in order to clarify that it is an abstract base class.This change is fairly innocuous in that it only affects star selectors (since they inherit from the base class), not users of those star selectors. I used a similar naming convention in DM-6077 making PSF determiners into tasks, which is what prompted the change.
            Hide
            rowen Russell Owen added a comment -

            Note that psfexStarSelector.py has a known (and reported as a separate ticket) undefined variable.

            Show
            rowen Russell Owen added a comment - Note that psfexStarSelector.py has a known (and reported as a separate ticket) undefined variable.
            Hide
            reiss David Reiss added a comment -

            I have been going through all of the changes and while I don't see any specific problems with any of the code or documentation changes, I have to admit that I am a bit confused as to why this was done. Is there a reason for using a registry rather than referring to or instantiating the individual classes themselves?

            Also, there's a question of the "constant" strings used for the registry - e.g. "objectSize". Should this perhaps be a class variable? Is there an easy way for someone who is (for example) using processCcd in obs_sdss to know which starSelectors are available, and what their names are in the registry?

            I have to admit that my question could simply be related to my lack of experience with and/or knowledge of this framework and the plans for it in the future.

            Show
            reiss David Reiss added a comment - I have been going through all of the changes and while I don't see any specific problems with any of the code or documentation changes, I have to admit that I am a bit confused as to why this was done. Is there a reason for using a registry rather than referring to or instantiating the individual classes themselves? Also, there's a question of the "constant" strings used for the registry - e.g. "objectSize". Should this perhaps be a class variable? Is there an easy way for someone who is (for example) using processCcd in obs_sdss to know which starSelectors are available, and what their names are in the registry? I have to admit that my question could simply be related to my lack of experience with and/or knowledge of this framework and the plans for it in the future.
            Hide
            rowen Russell Owen added a comment - - edited

            The justification for this change is given by RFC-183 and DM-6074. Related tasks should be kept together in registries, and if users are likely to want to swap them out, then a registry field is a good way to represent a subtask in a config.

            You raise an interesting point about class variables for registered names. It sounds like a good idea, but we've not done that before and we have existing registries, so I'd prefer to undertake that in a different ticket. This one was really just putting the registry back that was lost when star selectors were made into tasks (because at the time we didn't think tasks could be usefully used in registries), and then using that registry to specify the star selector subtask in two tasks.

            Show
            rowen Russell Owen added a comment - - edited The justification for this change is given by RFC-183 and DM-6074 . Related tasks should be kept together in registries, and if users are likely to want to swap them out, then a registry field is a good way to represent a subtask in a config. You raise an interesting point about class variables for registered names. It sounds like a good idea, but we've not done that before and we have existing registries, so I'd prefer to undertake that in a different ticket. This one was really just putting the registry back that was lost when star selectors were made into tasks (because at the time we didn't think tasks could be usefully used in registries), and then using that registry to specify the star selector subtask in two tasks.
            Hide
            reiss David Reiss added a comment -

            OK, I understand and agree. I still question how the new registry can be used to make related tasks easier to find. But other than that I only have a couple minor comments.

            Show
            reiss David Reiss added a comment - OK, I understand and agree. I still question how the new registry can be used to make related tasks easier to find. But other than that I only have a couple minor comments.
            Hide
            rowen Russell Owen added a comment -

            Thank you for the helpful review. I implemented all requested changes.

            I also took a cleanup pass on psfexPsfDeterminer, leaving a few undefined variables that I could not figure out (updating DM-5684 accordingly), and some more config override files for spaces around "="

            Show
            rowen Russell Owen added a comment - Thank you for the helpful review. I implemented all requested changes. I also took a cleanup pass on psfexPsfDeterminer, leaving a few undefined variables that I could not figure out (updating DM-5684 accordingly), and some more config override files for spaces around "="

              People

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

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins Builds

                  No builds found.