# calib_psfReserved is only defined when candidate reservation is activated

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
1
• Sprint:
DRP W16-7
• Team:
Data Release Production

#### 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.

#### Activity

Hide
Paul Price added a comment -

Merged to master.

Show
Paul Price added a comment - Merged to master.
Hide
Jim Bosch added a comment -

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

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

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

Show
Robert Lupton added a comment - I agree with Jim. This ticket is not a precedent for non-dynamic schema
Hide
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
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
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
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
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  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 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

#### People

Assignee:
Paul Price
Reporter:
Paul Price
Reviewers:
Russell Owen
Watchers:
Jim Bosch, Paul Price, Robert Lupton, Russell Owen