I very strongly support this proposal (thanks for saving me from having to push this myself in the future).
Your proposal to drop these flags in favor of retargeting sounds sensible to me. If for some reason that doesn’t work, I strongly believe we should avoid the having any options to load persisted results from a previous run of the same Task. That really complicates the provenance and makes it hard to track down bugs. In fact, as long as we avoid having config options that do that, I’m okay with any approach to the ambiguity here.
Upon further reflection, it may make sense to offer a “doIsr” flag that, if false, makes the task load “postIsrCCD” for image characterization. There are two issues at play:
First, ISR is a fairly stable operation. Users may prefer to run it once, then run characterization and calibration multiple times with different settings.
On the other hand, we don’t even really do ISR for some cameras. In particular:
For SDSS we load other files to generate the post-ISR data. To avoid duplicating data we don’t save this as “postISRCCD”, but regenerate it as needed.
For DECam we may want to perform our own ISR or load the results from the “community pipeline”.
There are several ways to support all this, including:
Make ISR an optional stage of ProcessCcdTask. For SDSS and DECam-with-community-pipeline use a special ISR stage that loads pre-existing data, and enable this flag. For DECam starting from raw data, use a different ISR stage.
Make ISR a separate command-line task and never have it be part of ProcessCcdTask. This requires handling SDSS and DECam differently, since the data product “postISRCCD” may not exist. I have heard the suggestion to handle this with special code in the camera camera mapper (e.g. when one requests “postISRCCD” in SDSS one runs the code that assembles the exposure from other data). DECam would require more care, since we may want to generate our own post-ISR data or use the community pipeline, but we could handle that by having two post-ISR data products. That requires adding a new config parameter to ProcessCcdTask.
Do both. Offer ISR as a command-line task and an optional component of ProcessCcdTask.
I have a slight preference for (1), but can see how (2) or (3) would also be appealing.
I have a fairly strong preference for (3), but my reading is that this (along with (1)) wouldn’t need a separate doIsr option, since that would be fully specified by whether the IsrTask subclass that’s being used is a real one or a dummy one.
This sort of warm-start behavior is a very valid feature request in general that I think we want to find better ways to support, but it also adds a lot of complication to provenance tracking (and config persistence in particular). I don’t want to get into to too many of the details right now, but the root of the problem is that using configuration to change how much processing to do (rather than just how to do it) conflicts with our goal of having a consistent configuration tree within each output repository; if you run once with one configuration, and then with another (even just to do a warm-start), you’re forced to use --clobber-config, which disables provenance tracking and opens the user up to a multitude of subtle bugs in the Butler. There are discussions underway in how to fix it, but until that’s addressed, configuration options like this are fraught with peril.u
And in this particular case, ISR is so fast that I don’t think re-running it will be considered a practical burden for users, so I think it’s much safer to just avoid supporting this request for now.
Thank you for the clear explanation of your concern about boolean flags. However, there is also a problem with proliferation of tasks: every task that is going to save provenance requires a new entry in all the obs_* mapper configuration files. That problem will go away when the butler is enhanced to support dataset prototypes (or whatever they will be called). Meanwhile it is not very practical to add a bunch of new tasks to the .paf files. I would rather live with boolean flags for now, despite the pain. Others may disagree.
@gpdf may want to comment further but one of the aims of the “superTask” work is to enable the provenance handling to be handled outside of the algorithmic code when the tasks are chained.
I don’t see where this concern is coming from; if we replace boolean doThing flags with no-op subtasks that we can retarget to in order to disable an option, the butler should be completely unaware of that change, since it only has datasets for top-level tasks.
Granted, our decision to make some subtasks into CmdLineTasks that can also be run independently does require some new mapper datasets (and it definitely complicates provenance - see my previous post on this). But the number of such CmdLineTask subtasks is sufficiently small that we shouldn’t distort our design for these Tasks just because Butler functionality isn’t available yet.
I decided to try this out by writing a null astrometry task. I found it quite messy. To simulate it well every public method must be overridden and it must do something sensible, then callers must be careful with the results. @jbosch and I discussed it and agreed that it’s not a good plan in general, though it may well be an excellent plan for some tasks, such as IsrTask that have simple, well defined null implementations (e.g. load postISRCCD).
@jbosch suggested we consider making lsst.pex.config.ConfigurableField allow a default of None as an easy way to disable tasks. I agree it sounds interesting, but want to make it a new ticket.
I’d like to move deep detection into ImageCharacterizationTask and am proposing just one new data product ‘icExp’ (and its associated background icExpBackground).
The current plan for ProcessCcdTask is as follows:
ProcessCcdTask(CmdLineTask)
IsrTask(CmdLineTask)
CharacterizeImageTask(CmdLineTask)
Repeat config.psfIterations times:
InstallSimplePsfModelTask: install a simple PSF model;
the default implementation uses a Gaussian whose sigma matches the existing PSF model
(if the exposure has one) else is take as a config parameter
SourceDetectionTask (just detect bright stars, 10-sigma limit; includes background estimation)
SourceDeblendTask (maybe unnecessary, but we should include it until/unless we can prove that)
SingleFrameMeasurementTask (just algorithms needed to select stars)
MeasurePsfTask (then go back to SourceDetectionTask.run and iterate as necessary)
RepairTask (remove cosmic rays)
CalibrateTask(CmdLineTask)
DetectAndMeasureTask (deep detection, measure and apply aperture correction)
SourceDetectionTask (detect to 5-sigma limit; re-estimate background)
SourceDeblendTask
SingleFrameMeasurementTask (all algorithms needed for Source table and calibration)
AstrometryTask
PhotoCalTask
I would like to move the deep detection run of DetectAndMeasureTask from CalibrateTask to CharacterizeImageTask. It seems a more natural fit because it can be argued as part of image characterization, and image characterization already performs similar operations.
The result is:
ProcessCcdTask (a CmdLineTask)
IsrTask (a CmdLineTask)
CharacterizeImageTask (a CmdLineTask)
Repeat config.psfIterations times:
InstallSimplePsfModelTask: install a simple PSF model;
the default implementation uses a Gaussian whose sigma matches the existing PSF model
(if the exposure has one) else is take as a config parameter
SourceDetectionTask (just detect bright stars, 10-sigma limit; includes background estimation)
SourceDeblendTask (maybe unnecessary, but we should include it until/unless we can prove that)
SingleFrameMeasurementTask (just algorithms needed to select stars)
MeasurePsfTask (then go back to SourceDetectionTask.run and iterate as necessary)
RepairTask (remove cosmic rays)
DetectAndMeasureTask (deeper detection, measure and apply aperture correction)
SourceDetectionTask (detect to 5-sigma limit; re-estimate background)
SourceDeblendTask
SingleFrameMeasurementTask (all algorithms needed for Source table and calibration)
CalibrateTask (a CmdLineTask)
AstrometryTask
PhotoCalTask
I was also tempted to eliminate CalibrateTask and have ProcessCcdTask call AstrometryTask and PhotoCalTask directly, because CalibrateTask does very little and the name is easily confused with CharacterizeImageTask. However, a requirement is to make calibration independently runnable as a command-line task, so I retained it.
To make CalibrateTask independently runnable requires that save the exposure that would be fed to it as a new dataset type.
Also, it is common to want to disable calibration, and if it is disabled then at present the resulting exposure is still saved as calexp. This is very useful because it means calexp is the best available version of the science exposure; tasks that take an image like that always can ask for calexp without needing to mess with the name.
I suggest saving the pre-calibration exposure as icExp and also saving it as calexp if calibration is disabled. That way CalibrateTask can be run later, if desired, but meanwhile other tasks can run without adding extra configuration.
I suggest not saving any other data products (such as the exposure after bright star detection and PSF modeling), because the changes are so small between that and after deeper detection.
I welcome suggestions for better names than CharacterizeImageTask and CalibrateTask. @ksk suggested the first one could be called MatchedFilterMeasurementTask. Names that include the steps quickly get too long (e.g. DetectMeasureAndModelPsfTask which still omits background modeling, repair and the final deeper detection and measurement pass).
I have heard complaints about the existing dataset names icSrc and src, but unless somebody can suggest good alternatives, I would rather keep them.
On the second (and greater) iteration will InstallSimplePsfModelTask use the “current best” PSF found during the previous run of the MeasurePsfTask step?
Is it a problem that astrometry is happening after the CharacterizeImageTask where the PSF is defined? What if we want to use an external catalog to select PSF stars? We’ll need decent astrometry to match those up.
Does the measurement task need the photometric zero point that PhotoCalTask produces? If so, wouldn’t we need measurement after the photometric calibration is run?
Can we run detection/measurement “SExtractor style”, i.e. independently on the command line? It’s not clear to me if DetectAndMeasureTask is a CmdLineTask.
Have you guys thought about adding a step (in the 2+ iteration) to subtract stars near the PSF stars to produce a cleaner PSF on the next round (this is a common step in packages like DAOPHOT)? Or is this all taken care of by ignoring pixels that have the detect mask set? The latter wouldn’t work well in crowded fields (because that would mask out most of the pixels in the image), but the formed could help.
I haven’t been following this discussion as closely as I would have liked, so apologies if you already answered some of these.
I will answer the ones I can. I hope others who know more about data characterization can handle the others.
Unfortunately not. That seemed like the right thing to do. Unfortunately I found that the PSF computation simply would not converge; successive iterations had wild swings in sigma, and with enough iterations the computation would fail. So instead I start each iteration using a simple Gaussian PSF model whose sigma matches the sigma of the most recently computed PSF model.
The intention is that CharacterizeImageTask can be run standalone, which is similar to running PSFEx followed by SExtractor. There are no current plans to make a standalone DetectAndMeasure.
The sigma of the simple Gaussian PSF model changes each time, and that model is used to repair the initial exposure and estimate background before performing detection, deblending and measurement.
Mostly the latter. Writing a simple task that read icExp and wrote src would not be difficult, though until we have the new butler it would require not persisting the config or adding more entries to various obs_* .paf files. An alternative I’m not keen on is to add a switch to CharacterizeImageTask that disables the initial detect/measure/fit PSF loop and call that our SExtractor. Switches tend to really complicate the task code, so we’re trying to avoid them.
@nidever Here’s a variant that might answer your issues:
The code that was called CharacterizeImageTask remains as it was (just iterate to find a PSF and background model). It acts like PSFEx and can be retargeted if you want to use some other technique to find PSF stars.
The deep detection pass in ProcessCcdTask is performed by a direct call to DetectAndMeasureTask, which becomes a command-line task and is like SExtractor
This means adding a few more more dataset types, which will be a naming headache, but keeps CharacterizeImageTask simpler.
First off, I’m not going to reply on dataset names yet. I think@npease has implemented some sort of aliasing scheme for dataset names that I think should inspire us to revisit the names of all of our datasets. But I don’t know enough about how it works to have an intelligent proposal right now, and I could easily be persuaded that this change should be done separately from @rowen’s work on restructuring the tasks.
Here’s my proposal for now:
I’m okay with moving the deep DetectAndMeasureTask run to CharacterizeImageTask.
CalibrateTask and CharacterizeImageTask should both write a full exposure dataset when they’re run independently. The same would be true of IsrTask, of course.
When run as part of ProcessCcdTask, config overrides there would cause IsrTask and CharacterizeImageTask not to write out their exposure datasets by default. I expect this to be the approach taken whenever we run data at large scale, and hence the only version we need to worry about when worrying about wasting disk space or I/O time.
Downstream processing would always depend on only calexp, and never icExp, and I don’t think that CharacterizeImageTask should ever write a calexp. That’s because while it’s important to be able to process an image without needing a reference catalog, there aren’t actually any downstream processing steps that can do anything with the image unless it has a Wcs and a Calib.
I think the right fix for this requires configuration features we don’t yet have: we need a way to configure each step in each iteration separately.
Of course, we could probably also do some work on the existing algorithms to make them more robust in this particular case, but that should be orthogonal to the refactoring. This approach sounds like it’s okay for now.
This is actually a very important tension I’d never really appreciated in previous debates about “reimplementing SExtractor with the stack”. I don’t think we want to think of any of these command-line tasks as a SExtractor replacement. And that’s because I think we want these tasks to be specific parts of an overall production, while the subtask components (e.g. DetectAndMeasureTask) are the reusable components, like SExtractor. I do think DetectAndMeasureTask essentially is SExtractor, albeit with a Python interface instead of a command-line one. And while it’d probably be very useful to have a command-line interface for DetectAndMeasureTask, I’m not sure it that interface should be a CmdLineTask, as that implies all sorts of Butler, workflow, and provenance connections that aren’t necessarily appropriate for one-off, ad-hoc use. It’s also not clear to me what relationship a command-line driver for a reusable subtask should have to the obs_* packages. I can imagine a close connection being very useful in some contexts, but being required to have an obs_* package before using such a tool is clearly not good in others.
In the eventual DRP pipeline I’m imagining, we’ll have to produce a preliminary PSF model without the aid of such a catalog in all cases before we can generate a final one later. In that context, all we’re talking about is that preliminary model - but it’s also all we have right now. If anyone can think up a case where they think it might be impossible to come up with any usable PSF model without the aid of an external catalog, I’d be very interested to discuss that.
It mostly does not need the zeropoint or other CalibrateTask outputs (since all outputs are in raw units). We do have a single algorithm (CModel galaxy fitting) that does use a zeropoint so it can make use of a Bayesian prior, but I’ve come to the conclusion that this was a mistake, in that I need to also provide a version of that algorithm that can work without a zeropoint in order to solve exactly this problem.
I’ve been thinking about how to fit it into the future pipeline sketch I’m working on, but I hadn’t planned to address it in this refactoring. But you make a very good point that adding a subtraction step to the iteration @rowen is adding here could be a natural way to try solve that problem. I also think that subtracting the extended profiles of bright stars fits in around the same place.
I agree. I had intended that CharacterizeImageTask write icExp. Then CalibrateTask would write calexp, which is the same as icExp if astrometric and photometric calibration are not run.
I wonder if it would be best to make DetectAndMeasureTask a separate step of ProcessCcdTask. The primary benefit is that it simplifies CharacterizeImageTask (which should probably be called MeasureInitialPsfTask) and makes it more modular. A drawback is that ProcessCcdTask is no longer a sequence of command-line tasks that can be run independently to produce the same result (unless we make DetectAndMeasureTask a command-line task and add some data products). I suppose this could also be taken as an argument for leaving the deep detection and measurement in the calibrate task. That might be best for now, with more refactoring later.
Thanks for the clarification. I had misunderstood this statement in your earlier post:
Now that (I think) I understand you better, I do still think it would be better to make CalibrateTask solely responsible for writing calexp, so calexp is not written when CalibrateTask is not run, and for ProcessCcdTask to warn in its config validatation when CalibrateTask is not being run but icExp is not being written. Does that make sense? I’m worried about us writing something that claims to be a calexp, but is actually an icExp, and actually isn’t useful for any of the downstream processing that needs a true calexp.
None of the options here jump out to me as obviously better than the others.
If we put the deep DetectAndMeasureTask run in CharacterizeImageTask, I’m worried that CharacterizeImageTask gets too complex in all the schemas and DetectAndMeasureTasks it needs to juggle, especially because that last DetectAndMeasureTask run is really only needed to feed CalibrateTask. But that may all be outweighed by the convenience it provides in allowing users who don’t want to run CalibrateTask to easily get a deep measurement catalog.
So if we think about how things fit together in a full pipeline, the deep DetectAndMeasureTask run clearly goes in CalibrateTask, since we’re doing it simply to feed those calibration routines (and, eventually, a bigger calibration routine like the one in meas_simastrom). I think I have a slight preference for this approach, but I’m willing to be overruled by @nidever if he’s concerned about not having an easy way to get to deep detection and measurement without going all the way to astrometric matching and photometric calibration.
The only case I’m aware of where the astrometry is already good is SDSS, for which the photometric calibration is also already good. If that is indeed the only such case, I’d be tempted to continue to have an SDSS-specific CalibrateTask to load and attach those to the exposure (it might now be more appropriate to do it here rather than earlier). Since CalibrateTask doesn’t have to do nearly as much as it used to, I think the downsides of having a camera-specific override would then be considerably lessened.
I had assumed that if we don’t have a reference catalog, we would just not run CalibrateTask itself at all (perhaps by running IsrTask and CharacterizeImageTask separately, perhaps by having a doCalibrate flag in ProcessCcdTask), but I’m not opposed to your proposal with two flags within CalibrateTask, and it does perhaps provide a better solution for users who do want deep measurements but don’t have a reference catalog.
Do all combinations of those two flags (e.g. doAstrometricCalibration=False, doPhotometricCalibration=True) make sense?
photoCalTask is not yet independent enough to do its own matching, though in the long run it surely will, since we want to be able to use separate astrometric and photometric catalogs. We can still have two flags if we want, but they won’t be fully independent yet.
Note that the current CalibrateTask has 4 flags: doAstrometry, doPhotoCal, requireAstrometry, requirePhotoCal. The latter two cause the task to raise an exception if the step fails This seems rather complex and I don’t see any code or config overrides that set the require flags true. Really these are tri-state: don’t do it, do it on a best-effort basis, do it and give up if it fails.