DM-27164: Add task to compute and persist VisitSummary tables #417

erykoff · 2020-10-15T17:29:22Z

No description provided.

parejkoj

Bunch of comments.

Thanks for writing the task topic type page from the start!

I don't see a test for this task. I think there's relevant data in testdata_jointcal, although maybe we shouldn't make that a requirement of this package. Unfortunately, I'm not sure we have another useful test dataset with prepackaged source catalogs. Rewriting run() so that it takes lists of real objects means that you could at least write a test with in-memory datasets (create a handful of WCS, PhotoCalib, etc. and pass them to run(), then check what you get out). Otherwise, it would be a fair bit more complicated with butler mocks.

I'm trying to decide whether it matters to create a test for a missing component (e.g. a detector in a visit with a missing SkyWcs). I don't think that would actually happen in practice, but I'm not positive.

python/lsst/pipe/tasks/postprocess.py

doc/lsst.pipe.tasks/tasks/lsst.pipe.tasks.postprocess.ConsolidateVisitSummaryTask.rst

parejkoj · 2020-10-22T19:38:36Z

python/lsst/pipe/tasks/postprocess.py

+        cat['visit'] = visit
+
+        if not self.isGen3:
+            bbox = lsst.geom.BoxI(lsst.geom.PointI(0, 0), lsst.geom.PointI(1, 1))


If you do stick with this if gen3 format, just put this inside the if below, with a comment that this is to speed up reads. But see my comment about reading composites via "_X" below: I don't think this is even necessary.

I still think this should go into the else: below, to make it more obvious that it's only for that. Also, maybe call it "gen2_read_bbox" or something, to distinguish it from the one you're going to persist.

python/lsst/pipe/tasks/postprocess.py

parejkoj · 2020-10-22T19:43:25Z

python/lsst/pipe/tasks/postprocess.py

+
+            sph_pts = wcs.pixelToSky(lsst.geom.Box2D(bbox).getCorners())
+            rec['raCorners'][:] = [sph.getRa().asDegrees() for sph in sph_pts]
+            rec['decCorners'][:] = [sph.getDec().asDegrees() for sph in sph_pts]


Ah, degrees. If we can't use an inherently unit-full storage type, I'm torn on whether degrees or radians is most appropriate here.

Replying to myself: it looks like most pipelines code does output degrees when it's a unit-less quantity, so I guess we'll go with that.

python/lsst/pipe/tasks/postprocess.py

erykoff · 2020-10-22T20:26:43Z

So the tests for this, such as they are, are run in ci_hsc_gen3, which are activated by adding the task to the pipeline in obs_subaru. That seems to be what is being done for the similar transform table tasks, and I was told that's what should be done.

python/lsst/pipe/tasks/postprocess.py

erykoff · 2020-11-10T22:57:31Z

I renamed run to _combineExposureMetadata to signify that this isn't really a true entry point. I think this is consistent with @TallJimbo 's comments: https://lsstc.slack.com/archives/C2JPT1KB7/p1605045131297300?thread_ts=1605042268.283600&cid=C2JPT1KB7

parejkoj

Thanks for the cleanups, and sorry it took me so long to get back to this. A handful more comments. I don't need to look at it again unless you think I should.

Please add a note to the either the Task or class docstring (I'm really torn on which is the best place, so I guess "why not both?") that testing of this happens in ci_hsc_gen3. If someone's looking to modify it, they'll want to know where the tests are.

parejkoj · 2020-11-10T20:55:57Z

python/lsst/pipe/tasks/postprocess.py

+        cat['visit'] = visit
+
+        if not self.isGen3:
+            bbox = lsst.geom.BoxI(lsst.geom.PointI(0, 0), lsst.geom.PointI(1, 1))


I still think this should go into the else: below, to make it more obvious that it's only for that. Also, maybe call it "gen2_read_bbox" or something, to distinguish it from the one you're going to persist.

parejkoj · 2020-11-10T23:35:57Z

python/lsst/pipe/tasks/postprocess.py

+
+            sph_pts = wcs.pixelToSky(lsst.geom.Box2D(bbox).getCorners())
+            rec['raCorners'][:] = [sph.getRa().asDegrees() for sph in sph_pts]
+            rec['decCorners'][:] = [sph.getDec().asDegrees() for sph in sph_pts]


Replying to myself: it looks like most pipelines code does output degrees when it's a unit-less quantity, so I guess we'll go with that.

parejkoj · 2020-11-10T23:56:00Z

python/lsst/pipe/tasks/postprocess.py

+            rec['psfIxy'] = shape.getIxy()
+            im = psf.computeKernelImage(bbox.getCenter())
+            # See https://github.com/lsst/meas_base/blob/
+            #     750bffe6620e565bda731add1509507f5c40c8bb/src/PsfFlux.cc#L112


Please note in this comment that this is for "why this is a reasonable way to compute psfArea", otherwise a random github link doesn't make sense. I think it's maybe better to say "see the calculation of measRecord in meas_base/src/PsfFlux.cc:112", so we aren't relying on a github link, too.

parejkoj · 2020-11-11T00:00:36Z

python/lsst/pipe/tasks/postprocess.py

+            rec.setBBox(bbox)
+            rec.setVisitInfo(visitInfo)
+            rec.setWcs(wcs)
+            rec.setPhotoCalib(photoCalib)


I'm not sure if this changed, but why aren't you persisting the Detector object here (I'll accept the argument that having the detector id trivially available in the outer metadata is useful)? There's a slot for it in ExposureRecord. I guess for now it's a constant thing, but eventually it has the possibility of changing during the survey.

parejkoj · 2020-11-11T00:04:08Z

python/lsst/pipe/tasks/postprocess.py

+            rec.setBBox(bbox)
+            rec.setVisitInfo(visitInfo)
+            rec.setWcs(wcs)
+            rec.setPhotoCalib(photoCalib)


Maybe have an explicit note here (or in the class docstring? I'm not sure) about why you aren't including the Psf, ApCorrMap, ValidPolygon, and TransmissionCurve in the output ExposureRecord? If someone does know what could be stored in it, they might want to add them here. I'm assuming it's a question of storage size for those things, but a note to that effect (probably here and in the docs both, as I think more) would be useful.

parejkoj · 2020-11-11T00:11:07Z

python/lsst/pipe/tasks/postprocess.py

+        schema.addField('raCorners', type='ArrayD', size=4,
+                        doc='Right Ascension of bounding box corners (degrees)')
+        schema.addField('decCorners', type='ArrayD', size=4,
+                        doc='Declination of bounding box corners (degrees)')


I do wish we had a better way to store these corners, but I can't think of one off hand.

parejkoj requested changes Oct 22, 2020

View reviewed changes

erykoff force-pushed the tickets/DM-27164 branch from a1412fb to 6eeb304 Compare October 27, 2020 18:16

parejkoj reviewed Nov 10, 2020

View reviewed changes

python/lsst/pipe/tasks/postprocess.py Show resolved Hide resolved

erykoff force-pushed the tickets/DM-27164 branch 2 times, most recently from 8e1999f to a62b0c4 Compare November 10, 2020 22:52

parejkoj approved these changes Nov 11, 2020

View reviewed changes

erykoff force-pushed the tickets/DM-27164 branch 2 times, most recently from c7598e7 to 3c3664d Compare November 12, 2020 20:54

erykoff added 2 commits November 12, 2020 16:10

Add ConsolidateVisitSummaryTask

560addd

Add ConsolidateVisitSummary to default pipeline.

264d5b7

erykoff force-pushed the tickets/DM-27164 branch from 3c3664d to 264d5b7 Compare November 13, 2020 00:10

erykoff merged commit 22db31c into master Nov 13, 2020

erykoff deleted the tickets/DM-27164 branch November 13, 2020 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-27164: Add task to compute and persist VisitSummary tables #417

DM-27164: Add task to compute and persist VisitSummary tables #417

erykoff commented Oct 15, 2020

parejkoj left a comment

parejkoj Oct 22, 2020

parejkoj Nov 10, 2020

parejkoj Oct 22, 2020

parejkoj Nov 10, 2020

erykoff commented Oct 22, 2020

erykoff commented Nov 10, 2020

parejkoj left a comment

parejkoj Nov 10, 2020

parejkoj Nov 10, 2020

parejkoj Nov 10, 2020

parejkoj Nov 11, 2020

parejkoj Nov 11, 2020

parejkoj Nov 11, 2020

DM-27164: Add task to compute and persist VisitSummary tables #417

DM-27164: Add task to compute and persist VisitSummary tables #417

Conversation

erykoff commented Oct 15, 2020

parejkoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erykoff commented Oct 22, 2020

erykoff commented Nov 10, 2020

parejkoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment