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

calib_psfReserved is only defined when candidate reservation is activated

    Details

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

      Description

      The schema should in general not be a function of whether particular features are enabled or disabled so that users can have confidence looking for columns. However, MeasurePsfTask only creates the calib_psfReserved column when reserveFraction > 0. This causes warnings when attempting to propagate flags from calibration catalogs to deep catalogs.

        Attachments

          Issue Links

            Activity

            Hide
            price Paul Price added a comment -

            Super-simple review, if you don't mind.

            price@price-laptop:~/LSST/pipe/tasks (tickets/DM-5385=) $ git sub-patch  
            commit 84c4ef518b658ab01ed55b3d10057e9ffbfe27a9
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Tue Mar 8 15:58:19 2016 -0500
             
                measurePsf: always add calib_psfReserved column to schema
                
                The schema, as far as possible, should not depend on configuration
                parameters.  Without calib_psfReserved in the schema, we can get
                errors/warnings when propagating flags from calib catalogs to deep
                catalogs.
             
            diff --git a/python/lsst/pipe/tasks/measurePsf.py b/python/lsst/pipe/tasks/measurePsf.py
            index 3f64ac6..9e3a5a5 100644
            --- a/python/lsst/pipe/tasks/measurePsf.py
            +++ b/python/lsst/pipe/tasks/measurePsf.py
            @@ -246,12 +246,10 @@ into your debug.py file and run measurePsfTask.py with the \c --debug flag.
                             doc=("Flag set if the source was actually used for PSF determination, "
                                  "as determined by the '%s' PSF determiner.") % self.config.psfDeterminer.name
                         )
            -            if self.config.reserveFraction > 0:
            -                self.reservedKey = schema.addField(
            -                    "calib_psfReserved", type="Flag",
            -                    doc=("Flag set if the source was selected as a PSF candidate, but was "
            -                         "reserved from the PSF fitting."))
            -                
            +            self.reservedKey = schema.addField(
            +                "calib_psfReserved", type="Flag",
            +                doc=("Flag set if the source was selected as a PSF candidate, but was "
            +                     "reserved from the PSF fitting."))
                     else:
                         self.candidateKey = None
                         self.usedKey = None
            

            Show
            price Paul Price added a comment - Super-simple review, if you don't mind. price@price-laptop:~/LSST/pipe/tasks (tickets/DM-5385=) $ git sub-patch commit 84c4ef518b658ab01ed55b3d10057e9ffbfe27a9 Author: Paul Price <price@astro.princeton.edu> Date: Tue Mar 8 15:58:19 2016 -0500   measurePsf: always add calib_psfReserved column to schema The schema, as far as possible, should not depend on configuration parameters. Without calib_psfReserved in the schema, we can get errors/warnings when propagating flags from calib catalogs to deep catalogs.   diff --git a/python/lsst/pipe/tasks/measurePsf.py b/python/lsst/pipe/tasks/measurePsf.py index 3f64ac6..9e3a5a5 100644 --- a/python/lsst/pipe/tasks/measurePsf.py +++ b/python/lsst/pipe/tasks/measurePsf.py @@ -246,12 +246,10 @@ into your debug.py file and run measurePsfTask.py with the \c --debug flag. doc=("Flag set if the source was actually used for PSF determination, " "as determined by the '%s' PSF determiner.") % self.config.psfDeterminer.name ) - if self.config.reserveFraction > 0: - self.reservedKey = schema.addField( - "calib_psfReserved", type="Flag", - doc=("Flag set if the source was selected as a PSF candidate, but was " - "reserved from the PSF fitting.")) - + self.reservedKey = schema.addField( + "calib_psfReserved", type="Flag", + doc=("Flag set if the source was selected as a PSF candidate, but was " + "reserved from the PSF fitting.")) else: self.candidateKey = None self.usedKey = None
            Hide
            jbosch Jim Bosch added a comment - - edited

            I philosophically disagree with the idea of insulating the schema from config changes; the whole point of having a dynamic schema is that we should be able to handle such changes with relative ease. There is admittedly a problem with linked configuration options, but that is a more general one I don't want to workaround by doing damage to the schema.

            That said, I'm a bit less concerned about this particular option. That's partly because setting the reserved flag to False for all stars is a natural limit as reserveFraction goes to zero (i.e. the presence of the field isn't confusing when the config option is set to a value that doesn't require it), and partly because the cost of the empty flag is just one bit per source. If either of those wasn't true, I'd fight a lot harder here.

            Show
            jbosch Jim Bosch added a comment - - edited I philosophically disagree with the idea of insulating the schema from config changes; the whole point of having a dynamic schema is that we should be able to handle such changes with relative ease. There is admittedly a problem with linked configuration options, but that is a more general one I don't want to workaround by doing damage to the schema. That said, I'm a bit less concerned about this particular option. That's partly because setting the reserved flag to False for all stars is a natural limit as reserveFraction goes to zero (i.e. the presence of the field isn't confusing when the config option is set to a value that doesn't require it), and partly because the cost of the empty flag is just one bit per source. If either of those wasn't true, I'd fight a lot harder here.
            Hide
            rowen Russell Owen added a comment -

            This looks good to me.

            Once this is merged, Paul Price and I would like to change CalibrateTask to raise an exception if any of the fields listed in icSourceFieldsToCopy is missing from icSrcCatalog. That way it is obvious if the code isn't going to do what the user asked. Jim Bosch are you comfortable with such a change?

            Note that I just merged DM-4692 to master, so CalibrateTask could be changed on this ticket, if desired.

            Show
            rowen Russell Owen added a comment - This looks good to me. Once this is merged, Paul Price and I would like to change CalibrateTask to raise an exception if any of the fields listed in icSourceFieldsToCopy is missing from icSrcCatalog . That way it is obvious if the code isn't going to do what the user asked. Jim Bosch are you comfortable with such a change? Note that I just merged DM-4692 to master, so CalibrateTask could be changed on this ticket, if desired.
            Hide
            rhl Robert Lupton added a comment -

            I agree with Jim. This ticket is not a precedent for non-dynamic schema

            Show
            rhl Robert Lupton added a comment - I agree with Jim. This ticket is not a precedent for non-dynamic schema
            Hide
            jbosch Jim Bosch added a comment -

            Russell Owen: changing to raise an exception on missing fields is fine with me.

            Show
            jbosch Jim Bosch added a comment - Russell Owen : changing to raise an exception on missing fields is fine with me.
            Hide
            price Paul Price added a comment -

            Merged to master.

            Show
            price Paul Price added a comment - Merged to master.

              People

              • Assignee:
                price Paul Price
                Reporter:
                price Paul Price
                Reviewers:
                Russell Owen
                Watchers:
                Jim Bosch, Paul Price, Robert Lupton, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel