Conversation
isullivan
force-pushed
the
tickets/DM-54625
branch
from
22683ba to
3486e8d
Compare
kfindeisen
left a comment
kfindeisen
left a comment
There was a problem hiding this comment.
I'm pretty sure the DECam/HSC versions of the new pipeline are broken -- they use the wrong ISR task. Unfortunately, I don't see a good fix, short of having separate LSST and non-LSST versions in _ingredients.
The other problem is that the contracts have been deferred to the instrument-specific tasks, and that means the ones for subtractImagesScore have slipped through the cracks.
| imports: | ||
| - location: $AP_PIPE_DIR/pipelines/_ingredients/ApPipeWithPreconvolution.yaml | ||
| tasks: | ||
| isr: | ||
| class: lsst.ip.isr.IsrTask | ||
| config: | ||
| file: $AP_PIPE_DIR/config/DECam/runIsrWithCrosstalk.py |
There was a problem hiding this comment.
This is incorrect, because ApPipeWithPreconvolution is based on ApPipeWithIsrTaskLSST, which isn't applicable to DECam. You could get around the problem by splicing _ingredients/ApPipeWithPreconvolution and DECam/ApPipe, but that would have the same contract/subset problems we get in ap_verify.
I suppose one question to ask is whether we actually need ApPipeWithPreconvolution for non-LSST cameras.
There was a problem hiding this comment.
This gets at the central problem I was trying to work around, but maybe it is too ugly of a kludge. I only wanted to make a single ApPipeWithPreconvolution and not duplicate it for ApPipeWithIsrTaskLSST and ApPipe, so for DECam and HSC I still inherit from ApPipeWithIsrTaskLSST but override the isr definition back to lsst.ip.isr.IsrTask. This is clearly misleading, but it does work. I would rather not do the splicing approach because of the contract and subset problems you mention, but dropping ApPipeWithPreconvolution for non-LSST cameras seems like a reasonable option.
I am willing to do the splicing approach if you think that would be best, drop ApPipeWithPreconvolution for the other cameras, or leave it as-is but add comments explaining what is going on and why. Your call.
There was a problem hiding this comment.
Oops, you're right, I don't know how I missed that. 🤦 Yes, I agree that overriding the ISR definition is probably best (IsrTaskLSST's connections seem to be a bit unstable, but hopefully IsrTask isn't changing anymore?).
I think highlighting the class change with a comment might be good enough. You might also add a unit test to guard against code drift (maybe that <instrument>/ApPipe and <instrument>/ApPipeWithPreconvolution have identical ISR task definitions?)
There was a problem hiding this comment.
Good idea. I have added a unit test that verifies that the ISR task configs are the same between ApPipe and ApPipeWithPreconvolution versions of the pipeline.
isullivan
force-pushed
the
tickets/DM-54625
branch
from
3486e8d to
d65ebc0
Compare
|
@kfindeisen I believe this PR is ready for re-review. |
kfindeisen
left a comment
kfindeisen
left a comment
There was a problem hiding this comment.
Looks good, assuming GitHub posts all my comments (I got some weird errors trying to record them).
| msg: "calibrateImage.exposure != analyzePreliminarySummaryStats.data" | ||
| - contract: calibrateImage.connections.ConnectionsClass(config=calibrateImage).stars_footprints.name == | ||
| getRegionTimeFromVisit.connections.ConnectionsClass(config=getRegionTimeFromVisit).dummy_visit.name | ||
| msg: "calibrateImage.stars_footprints != getRegionTimeFromVisit.dummy_visit" |
There was a problem hiding this comment.
Missing the contracts checking consistency of calibrateImage with subtractImagesScore -- or are the checks vs. subtractImages supposed to take care of that?
There was a problem hiding this comment.
No harm in adding those contracts as well.
| - location: $AP_PIPE_DIR/pipelines/_ingredients/ApPipeWithPreconvolution.yaml | ||
| tasks: | ||
| # Override the class for `isr`, which is set to `IsrTaskLSST` in the base | ||
| # pipeline in `_ingredients` |
There was a problem hiding this comment.
This comment seems to be missing from the DECam version.
| with self.subTest(file=str(precon_file)): | ||
| base_file = precon_file.dirname().join("ApPipe.yaml") | ||
| self.assertTrue(base_file.exists(), | ||
| msg=f"Expected sibling ApPipe.yaml next to {precon_file}.") |
There was a problem hiding this comment.
Might be better to report base_file directly, just in case there's a weird bug in defining it.
| self.assertEqual(precon_isr.task_class_name, base_isr.task_class_name, | ||
| msg=f"isr task class differs between ApPipe.yaml and " | ||
| f"ApPipeWithPreconvolution.yaml in {precon_file.dirname()}.") | ||
| self.assertTrue( | ||
| base_isr.config.compare(precon_isr.config, shortcut=False), | ||
| msg=f"isr task config differs between ApPipe.yaml and " | ||
| f"ApPipeWithPreconvolution.yaml in {precon_file.dirname()}." | ||
| ) |
There was a problem hiding this comment.
Ugh:
Task nodes are intentionally not equality comparable, since there are many different (and useful) ways to compare these objects with no clear winner as the most obvious behavior.
Maybe add a comment that you can't just do assertEqual(precon_isr, base_isr)?
| self.assertTrue( | ||
| base_isr.config.compare(precon_isr.config, shortcut=False), | ||
| msg=f"isr task config differs between ApPipe.yaml and " | ||
| f"ApPipeWithPreconvolution.yaml in {precon_file.dirname()}." | ||
| ) |
There was a problem hiding this comment.
For this, you could probably get away with assertEqual -- since the configs are being read directly from files, any floating-point handling should be deterministic.
isullivan
force-pushed
the
tickets/DM-54625
branch
from
d65ebc0 to
20f84c9
Compare
isullivan
force-pushed
the
tickets/DM-54625
branch
from
20f84c9 to
a001b5f
Compare
The Preconvolution pipelines have to redefine ISR for non-LSST cameras, so add tests to make sure that the configs are still consistent.
isullivan
force-pushed
the
tickets/DM-54625
branch
from
a001b5f to
2230013
Compare
2 participants
No description provided.