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(-)

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

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).

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.