Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: meas_base
-
Labels:
-
Story Points:1
-
Epic Link:
-
Sprint:Measurement-S14-4
-
Team:Data Release Production
Description
Some meas_base plugins still have old-style algorithm names.
Attachments
Activity
Just added GitHub PR to help with the review (linked from Issue page).
On the whole this looks just fine to me. The more complete transform in ForcedTransformedShapePlugin is nice. The new names seem fine.
I confess that I did not fully understand the changes to testForced.py (in particular refCat.table.defineCentroid("truth")) but I'm not worried about it.
My only substantive complaint is as follows:
The new process for making keys that apply to multiple fields seems somewhat clumsy and repetitive.
# Allocate x and y fields, join these into a single FunctorKey for ease-of-use.
|
xKey = schema.addField(name + "_x", type="D", doc="transformed reference centroid column",
|
units="pixels")
|
yKey = schema.addField(name + "_y", type="D", doc="transformed reference centroid row",
|
units="pixels")
|
self.centroidKey = lsst.afw.table.Point2DKey(xKey, yKey)
|
A loop reduces repetition (though one could argue about readability), and that might be enough for Point2, Extent2 and perhaps even Quadrupole:
keyList = [schema.addField("%s_%s" % (name, axis), type="D",
|
doc="transformed reference centroid, in %s" % (axis,),
|
units="pixels") for axis in ("x", "y")]
|
self.centroidKey = lsst.afw.table.Point2DKey(*keyList)
|
but it's not all that readable and it's not really enough for more complex common objects such as Box2I. I think some syntactic sugar, such as factory functions that construct keys for Box2, Point2, Extent2 and Quadrupole would be very helpful for making the user's intent clearer. For example:
self.centroidKey = lsst.afw.table.makePoint2Key(name, type="D", doc="transformed reference centroid",
|
units="pixels")
|
In addition to readability and reduced danger of typos, it also enforces the conventions for field name suffixes, though at the expense of a bit of "magic" and non-explicit field names.
I found myself unhappy with this level of boilerplate too, and while working on the issue, I added a note to DM-423 (a bit cryptic, looking at it today; will elaborate) to address this issue there. Generally speaking, while we put together syntactic sugar to allocate these fields in a consistent way in C++, we haven't yet made those interfaces easily usable from Python, and we definitely need to do that.
Russell, could you take a look? Most changes are trivial - just a renaming of some plugin algorithms to match new field name conventions. Only non-trivial changes are the addition of a psuedo-algorithm that's intended to fill the shape slot during forced measurement, by transforming the shape from the reference catalog. I also cleaned up the corresponding pseudo-algorithm for centroids.
All changes are in meas_base branch u/jbosch/
DM-1010.meas_base:u/jbosch/DM-1010 % git diff --stat master...u/jbosch/DM-1010python/lsst/meas/base/forcedMeasurement.py | 7 ++-python/lsst/meas/base/plugins.py | 86 ++++++++++++++++++++++++----tests/testForced.py | 35 ++++++-----tests/testGaussianCentroidBasic.py | 2 +-tests/testNaiveCentroidBasic.py | 2 +-tests/testNaiveFluxBasic.py | 4 +-tests/testPixelFlagsBasic.py | 4 +-tests/testPsfFluxBasic.py | 4 +-tests/testSdssCentroidBasic.py | 6 +-tests/testSdssShapeBasic.py | 2 +-tests/testSincFluxBasic.py | 4 +-tests/testSingleFrameMeasurement.py | 8 +--tests/testSkyCoordBasic.py | 4 +-13 files changed, 118 insertions(+), 50 deletions(-)