DM-31833: Move LoadDiaCatalogs to standalone pipelineTask #230
Conversation
There was a problem hiding this comment.
Please add MockLoadDiaCatalogsTask to tests/MockApPipe.yaml; otherwise there's no point in having it.
tests/test_testPipeline.py
Outdated
| MockSpatiallySampledMetricsTask, MockLoadDiaCatalogsTask | ||
| from lsst.daf.butler import Timespan | ||
| from lsst.pipe.base.utils import RegionTimeInfo | ||
| from lsst.sphgeom import UnitVector3d |
There was a problem hiding this comment.
I recommend sticking to the established (dependency) order, so that it's easier for future developers to see that the import already exists.
tests/test_testPipeline.py
Outdated
| points = [UnitVector3d.Z(), UnitVector3d.X(), UnitVector3d.Y()] | ||
| region = lsst.sphgeom.ConvexPolygon(points) |
There was a problem hiding this comment.
If you're making a dummy input, sphgeom.Circle has a default constructor so you don't have to import UnitVector3d. You get an empty region either way.
tests/test_testPipeline.py
Outdated
| @@ -347,9 +380,13 @@ def testMockDiaPipelineTask(self): | |||
| task = MockDiaPipelineTask(config=config) | |||
| pipelineTests.assertValidInitOutput(task) | |||
| result = task.run(pandas.DataFrame(), pandas.DataFrame(), afwImage.ExposureF(), | |||
| afwImage.ExposureF(), afwImage.ExposureF(), 42, 'k') | |||
| afwImage.ExposureF(), afwImage.ExposureF(), pandas.DataFrame(), | |||
| pandas.DataFrame(), pandas.DataFrame(), 'k', 42) | |||
There was a problem hiding this comment.
I think the DiaPipelineTask API has gotten to the point where we should be using keywords. 😬
There was a problem hiding this comment.
Same question about why this wasn't caught earlier. MockDiaPipelineTask allows idGenerator=None, but the original does not.
(And again, such a fix should have been a separate commit.)
There was a problem hiding this comment.
I'll clean up some of the mocks, but it looks like they were not updated when we removed ccdExposureIdBits. This would seem to be an argument in favor of removing test_testPipeline, since it doesn't necessarily catch changes to Task API's unless the Mocks in testPipeline.py are also updated.
isullivan
force-pushed
the
tickets/DM-31833
branch
from
cc0a050 to
0033313
Compare
isullivan
force-pushed
the
tickets/DM-31833
branch
from
0033313 to
60d8a8d
Compare
{Summary of changes. Prefix PR title with JIRA issue.}
sconsand/orstack-os-matrix)?ap_verify.pyon at least one of the standard datasets?For changes to metrics, the
print_metricvaluesscript fromlsst.verifywill be useful.