Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-240 meas_base plugins for CModel magnitudes
  3. DM-1224

Add FunctorKeys for ShapeletFunction and MultiShapeletFunction

    XMLWordPrintable

    Details

    • Type: Technical task
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: shapelet
    • Labels:
    • Sprint:
      Measurement-S14-4
    • Team:
      Data Release Production

      Description

      Add FunctorKey objects that know how to map ShapeletFunction and MultiShapeletFunction objects to records.

        Attachments

          Activity

          Hide
          jbosch Jim Bosch added a comment -

          Serge, up for a review of some semi-unfamiliar code? I've already overloaded pretty much all of the apps developers with big reviews recently, and I'd like to get your feedback on this pattern before I implement too many incarnations of it.

          For some background, these are convenience classes that inherit from afw::table::FunctorKey, a moderately recent interface for extracting and assigning complex C++ objects via a set of fields in a record (I expect this to replace the true compound fields in the near future, as those are a pain to map to SQL and we only have a limited number of compound types we can support that way).

          These two FunctorKeys delegate most of their work to other FunctorKeys already defined in afw, EllipseKey and ArrayKey. Just like regular Keys, one uses a FunctorKey by constructing it in advance from a Schema, and then using it repeatedly for more efficient access to multiple records.

          The C++ objects these FunctorKeys map to are the two main workhorse classes of the shapelet package, ShapeletFunction and MultiShapeletFunction. The mathematical details aren't important here, but it may help to know that a ShapeletFunction is just a polynomial expansion on top of an elliptical Gaussian, so it consists of an ellipse and a number of coefficients. There are also two common conventions for defining the polynomial basis, which I call "basis type" in the code.

          Please let me know if you won't be able to get to this in the next day or so, or if you have any questions.

          The changes are in the shapelet package, on branch u/jbosch/DM-1224. But to build it (if you want to), you'll need branch u/jbosch/DM-1222 (also in review).

          shapelet:u/jbosch/DM-1224 % git diff --stat master...u/jbosch/DM-1224
           include/lsst/shapelet.h                  |    1 +
           include/lsst/shapelet/FunctorKeys.h      |  229 ++++++++++++++++++++++++++++++
           include/lsst/shapelet/ShapeletFunction.h |    2 +-
           include/lsst/shapelet/constants.h        |   16 +++
           python/lsst/shapelet/shapeletLib.i       |   21 +++
           src/FunctorKeys.cc                       |  162 +++++++++++++++++++++
           src/ShapeletFunction.cc                  |    2 +-
           tests/testFunctorKeys.py                 |  102 +++++++++++++
           8 files changed, 533 insertions(+), 2 deletions(-)

          Show
          jbosch Jim Bosch added a comment - Serge, up for a review of some semi-unfamiliar code? I've already overloaded pretty much all of the apps developers with big reviews recently, and I'd like to get your feedback on this pattern before I implement too many incarnations of it. For some background, these are convenience classes that inherit from afw::table::FunctorKey , a moderately recent interface for extracting and assigning complex C++ objects via a set of fields in a record (I expect this to replace the true compound fields in the near future, as those are a pain to map to SQL and we only have a limited number of compound types we can support that way). These two FunctorKeys delegate most of their work to other FunctorKeys already defined in afw, EllipseKey and ArrayKey . Just like regular Keys , one uses a FunctorKey by constructing it in advance from a Schema , and then using it repeatedly for more efficient access to multiple records. The C++ objects these FunctorKeys map to are the two main workhorse classes of the shapelet package, ShapeletFunction and MultiShapeletFunction . The mathematical details aren't important here, but it may help to know that a ShapeletFunction is just a polynomial expansion on top of an elliptical Gaussian, so it consists of an ellipse and a number of coefficients. There are also two common conventions for defining the polynomial basis, which I call "basis type" in the code. Please let me know if you won't be able to get to this in the next day or so, or if you have any questions. The changes are in the shapelet package, on branch u/jbosch/ DM-1224 . But to build it (if you want to), you'll need branch u/jbosch/ DM-1222 (also in review). shapelet:u/jbosch/DM-1224 % git diff --stat master...u/jbosch/DM-1224 include/lsst/shapelet.h | 1 + include/lsst/shapelet/FunctorKeys.h | 229 ++++++++++++++++++++++++++++++ include/lsst/shapelet/ShapeletFunction.h | 2 +- include/lsst/shapelet/constants.h | 16 +++ python/lsst/shapelet/shapeletLib.i | 21 +++ src/FunctorKeys.cc | 162 +++++++++++++++++++++ src/ShapeletFunction.cc | 2 +- tests/testFunctorKeys.py | 102 +++++++++++++ 8 files changed, 533 insertions(+), 2 deletions(-)
          Hide
          jbosch Jim Bosch added a comment -

          Just added one more small commit to this branch, after discovering a bug, so the above diff has changed slightly.

          Show
          jbosch Jim Bosch added a comment - Just added one more small commit to this branch, after discovering a bug, so the above diff has changed slightly.
          Hide
          smonkewitz Serge Monkewitz added a comment -

          Jim - overall this looks great! The comments are top notch, the code is super easy to follow, and you've got a test case that serves as a good example should I ever need to revamp the ap package.

          I have just two very minor nitpicks:

          • In the addFields documentation for both ShapeletFunctionKey and MultiShapeletFunctionKey, you refer to coeffUnit, but the parameter is named coefficientUnit.
          • If there's any situation you can imagine where one might want to recover from an exception coming out of the addFields family of static methods, then it might be worth saying that only basic exception safety is provided (somewhere).

          and one question:

          • why does MultiShapeletFunctionKey contain a vector of shared pointers to keys, rather than just a vector of keys? It stood out that operator[] returns a shared ptr, when constituent keys are returned by const reference (ShapeletFunctionKey) or value (all the afw FunctorKey implementations I saw, e.g. (PointKey, ArrayKey, EllipseKey).
          Show
          smonkewitz Serge Monkewitz added a comment - Jim - overall this looks great! The comments are top notch, the code is super easy to follow, and you've got a test case that serves as a good example should I ever need to revamp the ap package. I have just two very minor nitpicks: In the addFields documentation for both ShapeletFunctionKey and MultiShapeletFunctionKey , you refer to coeffUnit , but the parameter is named coefficientUnit . If there's any situation you can imagine where one might want to recover from an exception coming out of the addFields family of static methods, then it might be worth saying that only basic exception safety is provided (somewhere). and one question: why does MultiShapeletFunctionKey contain a vector of shared pointers to keys, rather than just a vector of keys? It stood out that operator[] returns a shared ptr, when constituent keys are returned by const reference ( ShapeletFunctionKey ) or value (all the afw FunctorKey implementations I saw, e.g. ( PointKey , ArrayKey , EllipseKey ).
          Hide
          jbosch Jim Bosch added a comment -

          In the addFields documentation for both ShapeletFunctionKey and MultiShapeletFunctionKey, you refer to coeffUnit, but the parameter is named coefficientUnit.

          Fixed.

          If there's any situation you can imagine where one might want to recover from an exception coming out of the addFields family of static methods, then it might be worth saying that only basic exception safety is provided (somewhere).

          Fixed.

          why does MultiShapeletFunctionKey contain a vector of shared pointers to keys, rather than just a vector of keys? It stood out that operator[] returns a shared ptr, when constituent keys are returned by const reference (ShapeletFunctionKey) or value (all the afw FunctorKey implementations I saw, e.g. (PointKey, ArrayKey, EllipseKey).

          The reason for the vector of shared_ptr is Swig:

          • If a base class is wrapped using %shared_ptr, all its derived classes must be as well (and the base FunctorKey class is)
          • If a class is wrapped with %shared_ptr, I think it's impossible (or at least very difficult) to wrap a by-value std::vector.

          Basically, I'd generally slightly prefer to use const references in these situations, at least for non-trivial Keys, but the difficulty of wrapping a by-value vector pushed me back towards just doing this with shared_ptr, and I didn't feel the lack of consistency (though undesirable) is enough of a problem to go back and modify all the others.

          Show
          jbosch Jim Bosch added a comment - In the addFields documentation for both ShapeletFunctionKey and MultiShapeletFunctionKey, you refer to coeffUnit, but the parameter is named coefficientUnit. Fixed. If there's any situation you can imagine where one might want to recover from an exception coming out of the addFields family of static methods, then it might be worth saying that only basic exception safety is provided (somewhere). Fixed. why does MultiShapeletFunctionKey contain a vector of shared pointers to keys, rather than just a vector of keys? It stood out that operator[] returns a shared ptr, when constituent keys are returned by const reference (ShapeletFunctionKey) or value (all the afw FunctorKey implementations I saw, e.g. (PointKey, ArrayKey, EllipseKey). The reason for the vector of shared_ptr is Swig: If a base class is wrapped using %shared_ptr, all its derived classes must be as well (and the base FunctorKey class is) If a class is wrapped with %shared_ptr, I think it's impossible (or at least very difficult) to wrap a by-value std::vector. Basically, I'd generally slightly prefer to use const references in these situations, at least for non-trivial Keys, but the difficulty of wrapping a by-value vector pushed me back towards just doing this with shared_ptr, and I didn't feel the lack of consistency (though undesirable) is enough of a problem to go back and modify all the others.
          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:
            Serge Monkewitz
            Watchers:
            Jim Bosch, Serge Monkewitz
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                CI Builds

                No builds found.