Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: pipe_tasks
-
Labels:None
-
Story Points:1
-
Epic Link:
-
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.
Attachments
Issue Links
- is triggering
-
DM-5406 Require fields listed in icSourceFieldsToCopy to be present
- Done
Activity
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.
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.
I agree with Jim. This ticket is not a precedent for non-dynamic schema
Russell Owen: changing to raise an exception on missing fields is fine with me.
Merged to master.
Paul Price
added a comment - Merged to master. 
Super-simple review, if you don't mind.
price@price-laptop:~/LSST/pipe/tasks (tickets/DM-5385=) $ git sub-patchcommit 84c4ef518b658ab01ed55b3d10057e9ffbfe27a9Author: Paul Price <price@astro.princeton.edu>Date: Tue Mar 8 15:58:19 2016 -0500measurePsf: always add calib_psfReserved column to schemaThe schema, as far as possible, should not depend on configurationparameters. Without calib_psfReserved in the schema, we can geterrors/warnings when propagating flags from calib catalogs to deepcatalogs.diff --git a/python/lsst/pipe/tasks/measurePsf.py b/python/lsst/pipe/tasks/measurePsf.pyindex 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 = Noneself.usedKey = None