Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-1010

fix names of meas_base plugins to match new naming standards

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base
    • Labels:

      Description

      Some meas_base plugins still have old-style algorithm names.

        Attachments

          Activity

          Hide
          jbosch Jim Bosch added a comment -

          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-1010
           python/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(-)

          Show
          jbosch Jim Bosch added a comment - 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-1010 python/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(-)
          Hide
          jbosch Jim Bosch added a comment -

          Just added GitHub PR to help with the review (linked from Issue page).

          Show
          jbosch Jim Bosch added a comment - Just added GitHub PR to help with the review (linked from Issue page).
          Hide
          rowen Russell Owen added a comment -

          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.

          Show
          rowen Russell Owen added a comment - 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.
          Hide
          jbosch Jim Bosch added a comment -

          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.

          Show
          jbosch Jim Bosch added a comment - 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.
          Hide
          jbosch Jim Bosch added a comment -

          Merged to master.

          Show
          jbosch Jim Bosch added a comment - Merged to master.

            People

            Assignee:
            jbosch Jim Bosch
            Reporter:
            jbosch Jim Bosch
            Reviewers:
            Russell Owen
            Watchers:
            Jim Bosch, Russell Owen
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.