DM-9599: Support concatenation of Transforms #214

kfindeisen · 2017-04-07T16:54:55Z

This PR implements Transform::of in terms of ast::prepend. It deviates slightly from the DM-9599 spec in that the C++ code checks for compatible endpoints at compile time; this turned out to be less work than the original run-time-only design because it reduces the number of overloads that need to be wrapped in pybind11. The need to explicitly consider one overload per endpoint implementation class already complicates things enough.

r-owen

This is all I have time for now. I'll finish later. I hope the suggested doc rewording seems at least somewhat reasonable and you can come up with a version we both like.

r-owen · 2017-04-08T01:45:27Z

include/lsst/afw/geom/Transform.h

+     * @param before the Transform to apply before this one
+     * @returns a Transform that first applies `before` to its input, and then
+     *          this Transform to the result. Its inverse shall first apply the
+     *          inverse of this Transform, then the inverse of `before`.


I'm finding this hard to follow. The template parameter LastEndpoint name seems backwards to me, which makes the rest of it harder for me to follow.

See if something like the following works for you:

I strongly suggest you name the argument first, just like ast::Mapping::of. Commonality is a huge help here. (I realize I would naturally prefer first since that's what I chose for astshim. If you are very keen for before we could consider changing it in both places. But I find first clear and concise.)

Then document could look something like this:

Concatenate two transforms to produce a new transform C(x) = this(first(x))

The resulting transform has the "from" endpoint of first (FromFirstEndpoint), and the "to" endpoint of this transform (ToEndpoint). The other two endpoints, where the transforms are joined, are required to be identical: thus the "to" transform of first must be the same as "from" endpoint of this transform: FromEndpoint. The compiler assures that the classes are the same, but a runtime check is required to make sure the number of axes match.

template<class FirstFromEndpoint> Template<class FirstFromEndpoint, ToEndpoint> of(Template<FirstFromEndpoint, FromEndpoint> const & first)

r-owen · 2017-04-08T01:45:38Z

include/lsst/afw/geom/Transform.h

@@ -60,7 +60,7 @@ since the base and current frames in the FrameSet can be checked against by the
 because data must be copied when converting from LSST data types to the type used by astshim,
 so it didn't seem worth the bother.
 */
-template <typename FromEndpoint, typename ToEndpoint>
+template <class FromEndpoint, class ToEndpoint>


r-owen · 2017-04-08T01:46:17Z

include/lsst/afw/geom/Transform.h

+     *          inverse of this Transform, then the inverse of `before`.
+     *
+     * @throws InvalidParameterErrror Thrown if `this->getFromEndpoint()` does
+     *                                not match `before.getToEndpoint()`.


I find this a bit confusing because the compiler makes sure the classes match. But this does not necessarily mean the number of axes match. I tried to say something about that in my suggested documentation above, and that might suffice, but I wonder if this @throws description could also be clearer.

r-owen · 2017-04-08T01:56:24Z

include/lsst/afw/geom/Transform.h

+     * More than two Transforms can be combined in series. For example:
+     *
+     *     auto pixelsToSky = pupilToSky.of(fpToPupil)
+     *                                  .of(pixelsToFp);


I'm not sure it is worth changing, but I usually name transforms using from instead of to because a_values = aFromB(b_values). When worded that way this example will read much more naturally.

r-owen

Very nice overall, but I do have some requested changes.

r-owen · 2017-04-12T18:25:35Z

python/lsst/afw/geom/transform/transform.cc

+template <class ExtraEndpoint, class FromEndpoint, class ToEndpoint, class PyClass>
+void declareOf(PyClass &cls) {
+    using PrefixClass = Transform<ExtraEndpoint, FromEndpoint>;
+    using CalledClass = Transform<FromEndpoint, ToEndpoint>;


I confess I have no idea which of these two names is for which of the two transforms (first or second/this). Please consider clearer names, such as FirstTransform, SecondTransform and FinalTransform (or MergedTransform).

(I don't recommend FirstClass and SecondClass because those terms are usually used for levels of quality)

Is

(FinalTransform (SecondTransform::*)(FirstTransform const &) const) & SecondTransform::template of<ExtraEndpoint>

clear?

Much nicer. Thanks!

r-owen · 2017-04-12T18:34:14Z

python/lsst/afw/geom/transform/transformContinued.py

@@ -33,10 +35,20 @@ def getJacobian(self, x):
                    self.getFromEndpoint().getNAxes())
    return matrix

+
+def of(self, before):
+    if isinstance(before.getToEndpoint(), type(self.getFromEndpoint())):


I'm surprised you need additional wrapping for of (and suggested explaining why as a code comment to the wrapper file). However, I'm betting you do. But given that you do need a wrapper, I am surprised by this test. Naively I would expect if before.getToEndpoint() == self.getFromEndpoint().

If you do want to test type instead, for some reason, I have a few suggestions:

Consider if type(before.getToEndpoint()) == type(self.getFromEndpoint()) because isinstance is asymmetrical and will be true even if the left hand side is a subclass of the right hand side; that seems unsafe for future endpoint classes.

Consider catching mismatched # of axes here, instead of in the C++ code, in order to give more readable error messages.I haven't tested to see what the message is, but I worry because the C++ class names don't match the Python all that well.

Using the equality test didn't occur to me; that is cleaner.

The original code had type(...) == type(...), but flake8 didn't like it.

I'm glad equality works.

I'm surprised that flake8 objected to type(...) == type(...). It's perfectly valid, though one rarely should need to do it. For fun I made a small snippet and flake8 had no objections to it:

a = "string" b = "another" assert type(b) == type(a)

r-owen · 2017-04-12T18:36:02Z

python/lsst/afw/geom/transform/transform.cc

+    using PrefixClass = Transform<ExtraEndpoint, FromEndpoint>;
+    using CalledClass = Transform<FromEndpoint, ToEndpoint>;
+    using FinalClass = Transform<ExtraEndpoint, ToEndpoint>;
+    cls.def("_of", (FinalClass (CalledClass::*)(PrefixClass const &) const) &


Please explain as a code comment the fact that _of is used for of in the python continuation file, and why that is needed -- why isn't the pybhind11 C++ wrapper sufficient?

r-owen · 2017-04-12T18:53:16Z

src/geom/Transform.cc

+template <class FromEndpoint, class ToEndpoint>
+template <class LastEndpoint>
+Transform<LastEndpoint, ToEndpoint> Transform<FromEndpoint, ToEndpoint>::of(
+        Transform<LastEndpoint, FromEndpoint> const &before) const {


Reminder: if you rename the type parameters in the header file, please also rename them here.

r-owen · 2017-04-12T18:54:57Z

tests/test_transform.py

-                                          err_msg=msg)
-            np.testing.assert_array_equal(frameSet.tranForward(rawInArray),
-                                          invFrameSet.tranInverse(rawInArray),
-                                          err_msg=msg)


Nice improvement. Thanks for doing this.

r-owen · 2017-04-12T18:57:01Z

tests/test_transform.py

+        toName : string
+            the prefix of the ending endpoint for the final, concatenated
+            Transform
+        """


I don't think it's a good idea to mix Doxygen and numpydoc formatting in the same file. Please either update the other doc strings to numpydoc or use Doxygen formatting here.

Ok, I've modernized to numpydoc.

r-owen · 2017-04-12T19:00:57Z

tests/test_transform.py

+            polyMap = makeTwoWayPolyMap(2, 3)
+            transform1 = transform1Class(polyMap)
+            transform2 = transform2Class(polyMap)
+            transform = transform2.of(transform1)


I suggest rewriting this test so that only the transform2.of is expected to fail, and put only that statement into the with block. That way you can be sure that a failure occurs in the statement you care about, and that a coding error in the other statements will show up as an unexpected exception.

If I understand correctly you are testing # of axes mismatch, and such a test is only sensible if midName == "Generic". if so I would do something like this:

# Verify that trying to join the same type of endpoint but mismatched # numbers of axes, fails. Note that only GenericEndpoints can have # variable numbers of axes. if midName == "Generic": polyMap = makeTwoWayPolyMap(2, 3) transform1 = transform1Class(polyMap) transform2 = transform2Class(polyMap) with self.assertRaises(InvalidParameterError): transform = transform2.of(transform1)

r-owen · 2017-04-12T19:03:59Z

tests/test_transform.py

+                transform1 = transform1Class(polyMap)
+                polyMap = makeTwoWayPolyMap(3, 3)
+                transform2 = transform1Class(polyMap)
+                transform = transform2.of(transform1)


Again, please rewrite so only the transform2.of statement is in the with statement.

In this case I think you are trying to test type mismatch (whether or not the # of axes matches, though you already handled mismatched # of axes above)), which is a great idea, but I think to do that properly you should iterate over the # of valid axes for the two transforms you are computing. And again, a code comment explaining what you are testing would be helpful.

Why is checking all numbers of axes relevant here?

If you are testing type mismatch then I think the main thing is to test at least one set of valid axes for a given endpoint type. Also, the #s of axes should match if that is allowed for the given types, so you know you are testing type mismatch instead of # of axes mismatch.

However, may be easier and more thorough to cycle through all valid #s of axes for each endpoint type. One could imagine that code testing both type mismatch and axis mismatch, e.g. cycle through all choices and if types differ or #s of axes differ then assert that of will throw an exception.

r-owen · 2017-04-12T19:20:27Z

tests/test_transform.py

+        """Test that both conventions for chaining Transform*To*.of give
+        the same result
+        """
+        # Getting order of operations wrong should throw


Please delete this comment. Or explicitly add a test for it right after the comment, if you prefer.

r-owen

Looks great! Just a few minor comments.

r-owen · 2017-04-13T19:22:42Z

python/lsst/afw/geom/transform/transform.cc

+    // Need Python-specific logic to give sensible errors for mismatched Transform types
+    cls.def("_of", (FinalTransform (SecondTransform::*)(FirstTransform const &) const) &
+                           SecondTransform::template of<ExtraEndpoint>,
+            "before"_a);


Please use "first"_a

r-owen · 2017-04-13T19:23:05Z

python/lsst/afw/geom/transform/transform.cc

+    using FirstTransform = Transform<ExtraEndpoint, FromEndpoint>;
+    using SecondTransform = Transform<FromEndpoint, ToEndpoint>;
+    using FinalTransform = Transform<ExtraEndpoint, ToEndpoint>;
+    // Need Python-specific logic to give sensible errors for mismatched Transform types


r-owen · 2017-04-13T19:24:45Z

python/lsst/afw/geom/transform/transform.cc

@@ -81,7 +92,14 @@ void declareTransform(py::module &mod, std::string const &fromName, std::string
    cls.def("tranInverse", (FromArray (Class::*)(ToArray const &) const) & Class::tranInverse, "array"_a);
    cls.def("tranInverse", (FromPoint (Class::*)(ToPoint const &) const) & Class::tranInverse, "point"_a);
    cls.def("getInverse", &Class::getInverse);
+    // Need some extra handling of ndarray return type in Python


This is helpful but specifics would be appreciated, e.g. "Need some extra handling of ndarray return type in Python to avoid dimensions of length 1 from being deleted, leaving a 1-dimensional array or scalar"

(if a 1 x 1 matrix is compressed to numpy scalar? if not, delete the last two words)

r-owen · 2017-04-13T19:25:32Z

python/lsst/afw/geom/transform/transformContinued.py

+
+def of(self, before):
+    if before.getToEndpoint() == self.getFromEndpoint():
+        return self._of(before)


Please use first instead of before

r-owen · 2017-04-13T19:37:41Z

tests/test_transform.py

+                self.goodNaxes[midName])
+            for nIn in self.goodNaxes[fromName]:
+                for nMid in joinNaxes:
+                    for nOut in self.goodNaxes[midName]:


Did you mean self.goodNaxes[toName] here? On the other hand, the test works so maybe not.

I'm keeping this subtest simple by making both Transforms the same type, but I agree that it's hard to spot. Any suggestions for how to make it more obvious?

Perhaps add outName = midName # to keep the test simple and then use outName when referring to the output of the second transform? Or at least add a comment explaining what is going on.

This is a template method, which substantially complicates both the LSST convention of explicitly instantiating all templates and the pybind11 wrapper. However, I don't see a better way to do things.

This brings the `Transform` code in compliance with style guide rule 3-7, since endpoints will never be primitive types.

kfindeisen requested a review from r-owen April 7, 2017 16:54

r-owen reviewed Apr 8, 2017

View reviewed changes

r-owen requested changes Apr 12, 2017

View reviewed changes

kfindeisen force-pushed the tickets/DM-9599 branch from 8957d5a to 2ba7775 Compare April 12, 2017 19:59

Move AST test utilities to astshim.

8f0139d

kfindeisen force-pushed the tickets/DM-9599 branch from 2ba7775 to 45fa2ca Compare April 13, 2017 18:31

r-owen approved these changes Apr 13, 2017

View reviewed changes

kfindeisen added 4 commits April 13, 2017 13:21

Add Transform::of.

90e6c16

This is a template method, which substantially complicates both the LSST convention of explicitly instantiating all templates and the pybind11 wrapper. However, I don't see a better way to do things.

Replace typename with class in template parameter lists.

3f0a452

This brings the `Transform` code in compliance with style guide rule 3-7, since endpoints will never be primitive types.

Fix typo in unit tests.

bf5f987

Modernize docstrings.

d13b270

kfindeisen force-pushed the tickets/DM-9599 branch from 45fa2ca to d13b270 Compare April 13, 2017 20:21

kfindeisen merged commit d13b270 into master Apr 13, 2017

kfindeisen deleted the tickets/DM-9599 branch August 22, 2018 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-9599: Support concatenation of Transforms #214

DM-9599: Support concatenation of Transforms #214

kfindeisen commented Apr 7, 2017

r-owen left a comment

r-owen Apr 8, 2017

r-owen Apr 8, 2017

r-owen Apr 8, 2017

r-owen Apr 8, 2017

r-owen left a comment

r-owen Apr 12, 2017

kfindeisen Apr 13, 2017

r-owen Apr 13, 2017

r-owen Apr 12, 2017

kfindeisen Apr 13, 2017 •

edited

r-owen Apr 13, 2017

r-owen Apr 12, 2017

r-owen Apr 12, 2017

r-owen Apr 12, 2017

r-owen Apr 12, 2017

kfindeisen Apr 13, 2017

r-owen Apr 13, 2017

r-owen Apr 12, 2017

r-owen Apr 12, 2017

kfindeisen Apr 13, 2017

r-owen Apr 13, 2017

r-owen Apr 12, 2017

r-owen left a comment

r-owen Apr 13, 2017

r-owen Apr 13, 2017

r-owen Apr 13, 2017

r-owen Apr 13, 2017

r-owen Apr 13, 2017

kfindeisen Apr 13, 2017

r-owen Apr 13, 2017

DM-9599: Support concatenation of Transforms #214

DM-9599: Support concatenation of Transforms #214

Conversation

kfindeisen commented Apr 7, 2017

r-owen 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

r-owen 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

kfindeisen Apr 13, 2017 • edited

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

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

r-owen 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

Choose a reason for hiding this comment

kfindeisen Apr 13, 2017 •

edited