DM-39858: Integrate new CalibrateImageTask with AP pipeline #147
Conversation
parejkoj
force-pushed
the
tickets/DM-39858
branch
from
c0a17da
to
bcf4b0a
Compare
parejkoj
force-pushed
the
tickets/DM-39858
branch
2 times, most recently
from
c6fa001
to
5953535
Compare
There was a problem hiding this comment.
The DM-31047 hacks appear to have been cargo-culted as-is, despite the fact that, as a task introduced after RFC-775, calibrateImage shouldn't have any DRP bias in its obs_ configs. Then again, I can't find any configs for it in obs_lsst... what's going on?
Would it be better to put all the possibilities in the same ingredients files, and then have separate
XCalibrateImage.yamlfor the instrument-specific ones? Suggestions, please!
Sorry, but even after looking at the changes, I still don't understand this question. What are the "possibilities" you mention?
There was a problem hiding this comment.
Does this really belong in ap_verify, and not in obs_lsst? Since this is a new task, there's no reason we can't make the config RFC-775-compliant from the beginning.
There was a problem hiding this comment.
Great, that's just what I was hoping to hear! I wasn't sure if we wanted to fix everything at once, or if we could migrate things one at a time. I'll happily move it over to obs_lsst (because it really is the generic settings you always want for imsim).
| # blow away all the configs that are set in this file. | ||
| # To update a pipeline config prior to DM-35504, you MUST put it in either, | ||
| # e.g., $AP_PIPE_DIR/config/$CAMERA/someTask.py, or in a camera-specific, | ||
| # pipeline, e.g., $AP_PIPE_DIR/pipelines/$CAMERA/ApTemplate.yaml. |
There was a problem hiding this comment.
| # pipeline, e.g., $AP_PIPE_DIR/pipelines/$CAMERA/ApTemplate.yaml. | |
| # pipeline, e.g., $AP_PIPE_DIR/pipelines/$CAMERA/ApTemplateCalibrateImage.yaml. |
There was a problem hiding this comment.
Just to check: this warning re: DM-35504 still applies here? If we're adding a CalibrateImage override file to obs_lsst/config/imsim (which we want, because they're very specific to imsim), does follow the spirit comment given here, go against it, or is it orthogonal to this?
There was a problem hiding this comment.
Yes, it still applies. The comment is about the pipeline as a whole, and we still have to hack around some overrides for e.g. makeWarp.
|
Also, I just noticed that the commit message mentions |
For example, I could include this line in the ingredients file: and then in the old files exclude it while the new files exclude |
I wrote it that way because ap_verify is the way we're going to start testing the new task. I can reword it to take out that reference? |
parejkoj
force-pushed
the
tickets/DM-39858
branch
from
5953535
to
2e7c031
Compare
Got it. No, I'd rather not; part of my argument on DM-40210 was that the |
These ISR overrides were either already set in obs_lsst (the two booleans), or have been set there (`connections.newBFKernel`) on this ticket.
parejkoj
force-pushed
the
tickets/DM-39858
branch
4 times, most recently
from
9da947d
to
44f205d
Compare
| @@ -40,6 +40,8 @@ tasks: | |||
| assembleStaticSkyModel.doSelectVisits: True | |||
| connections.outputCoaddName: parameters.coaddName | |||
|
|
|||
| # TODO DM-40389: Remove this entire block once we are using calibrateImage in | |||
| # the base _ingredients/ApTemplate.yaml. | |||
There was a problem hiding this comment.
This block is redefining the singleFrameAp subset after the single-frame tasks are spliced in from HyperSuprimeCam/ProcessCcd.yaml. Why would it become obsolete once that's implemented using calibrateImage?
There was a problem hiding this comment.
Because those single frame tasks in HyperSuprimeCam/ProcessCcd.yaml will be gone: they're overriding tasks that will no longer exist.
This setup lets us run both the old (characterizeImage+calibrate) and new (calibrateImage) pipelines as separate runs, so that we can directly compare the new and old versions.
parejkoj
force-pushed
the
tickets/DM-39858
branch
from
da8862f
to
c053397
Compare
No description provided.