DM-9595: Allow Transform to return its inverse #192

kfindeisen · 2017-03-17T02:37:01Z

This PR implements Transform::getInverse(), adds a Python-only equality test for Endpoint, and updates the style of the Transform code to match the output of clang-format (mostly changing whitespace around &).

jdswinbank · 2017-03-17T12:12:27Z

tests/test_transform.py

+        outArray = transform.tranForward(inArray)
+        rawOutArray = toEndpoint.dataFromArray(outArray)
+        # Assertions must work with both lists and numpy arrays
+        # TODO replace with assertFloatsAlmostEqual once DM-9707 is fixed


My suggestion would be to avoid embedding TODOs like this in the code — we have literally hundreds of them scattered across the codebase, and nobody has time to regularly audit old code to see what actually needs to be done. Perhaps you could file a ticket on JIRA to do this work, and mark it as triggered by DM-9707? (Obviously there are thousands of tickets in JIRA which need keeping track of, but at least that means we only need to track a single source.)

That seems reasonable for DM-9707. Note that the similar comments for DM-9826 represent work to be done as part of DM-9826, they're just there to justify omissions to the reviewer.

Consider deleting the note and calling the code done. Thenumpy.testing.assert_... functions are excellent and I see no reason to ever replace your usage of them.

@r-owen does that also apply to your note about replacing np.allclose?

np.allclose should be replaced with np.testing.assert_allclose. Do you want to do that while working on the ticket or hand it back to me to do. I didn't know about the np.testing asserts when I wrote the code.

I'll do it, and remove all mentions of DM-9707.

r-owen

Overall it looks excellent to me. I had two substantive suggestions:

Please construct the inverse transform using a frame set instead of a mapping, in order to preserve information.
Consider implementing DM-9826 as part of this ticket, because it is trivial and provides useful capability for testing getInverse.

r-owen · 2017-03-17T15:20:16Z

python/lsst/afw/geom/endpoint.cc

        auto frame = self.makeFrame();
        return frame->copy();
    });
 }

+// Endpoint comparison not needed(?) in C++


If making this available in C++ would be little extra work please do so. Otherwise please update the comment to explain that it's not being made available in pure C++ because it is too much work and is primarily intended for unit tests (or something like that). The current note, with its question mark, makes it feel unfinished.

Agreed, I've already decided that it definitely should not be present in C++ (it's redundant with the type system).

r-owen · 2017-03-17T15:24:23Z

tests/test_endpoint.py

+             afwGeom.SpherePointEndpoint()]
+        endpoints2 = [afwGeom.GenericEndpoint(n) for n in range(1, 6)] + \
+            [afwGeom.Point2Endpoint(), afwGeom.Point3Endpoint(),
+             afwGeom.SpherePointEndpoint()]


Consider adding a function or method that returns this list. Then call it in each for loop below. I can imagine other tests taking advantage of this as well (now or in the future).

It could even go into the existing geom/testUtils.py module so that test_endpoint.py could (eventually) use it.

r-owen · 2017-03-17T15:26:13Z

tests/test_transform.py

+                    nIn,
+                    "{}, Map={}, nIn={}, nOut={}".format(
+                        baseMsg, "Invertible", nIn, nOut))
+                # TODO: add more test cases once DM-9826 resolved


I suggest implementing that ticket as part of this one; it should be trivial and the result will be a more complete and coherent solution, since it is required -- or at least very useful -- for fully testing the inverse.

Another option is to implement that ticket first, then this one. But that seems needlessly complicated for such a simple thing.

I was trying to be better about organizing work, avoiding scope creep, etc., but fine. I will ask for a re-review of those changes, though.

r-owen · 2017-03-17T15:29:25Z

tests/test_transform.py

+        outArray = transform.tranForward(inArray)
+        rawOutArray = toEndpoint.dataFromArray(outArray)
+        # Assertions must work with both lists and numpy arrays
+        # TODO replace with assertFloatsAlmostEqual once DM-9707 is fixed


Consider deleting the note and calling the code done. Thenumpy.testing.assert_... functions are excellent and I see no reason to ever replace your usage of them.

r-owen · 2017-03-17T15:34:47Z

tests/test_transform.py

+                         inverse.getToEndpoint(), msg=msg)
+        self.assertEqual(transform.getToEndpoint(),
+                         inverse.getFromEndpoint(), msg=msg)
+        # TODO: test hasTranForward and hasTranInverse once DM-9826 resolved


Again, if you implement that now you can provide a more complete solution

r-owen · 2017-03-17T15:57:11Z

src/geom/Transform.cc

@@ -105,16 +105,22 @@ typename FromEndpoint::Array Transform<FromEndpoint, ToEndpoint>::tranInverse(
 }

 template <typename FromEndpoint, typename ToEndpoint>
+Transform<ToEndpoint, FromEndpoint> Transform<FromEndpoint, ToEndpoint>::getInverse() const {
+    std::shared_ptr<ast::Mapping> inverse = _frameSet->getInverse();


I strongly suggest using the FrameSet constructor of Transform to construct the inverse, because it retains more information: if the Transform was originally constructed with a FrameSet, the inverse retains all the frames and individual transforms of the original.

I see two simple ways to do this:

Use dynamic_pointer_cast to cast the value returned by _frameSet.getInverse() to std::shared_ptrast::FrameSet. That should never fail, but of course you will have to check it anyway.

Make the inverse manually by copying the frame set swapping the BASE and CURRENT frame pointers:

auto inverse = _frameSet->copy(); inverse->setBase(_frameSet->getCurrent()); inverse->setCurrent(_frameSet->getBase());

Hmm... is there a way to unit test for the information that's currently missing?

Sure...create a FrameSet and add some frames and connecting mappings. Then look for the frames after constructing the Transform and retrieving the FrameSet from it.

r-owen · 2017-03-17T16:04:37Z

tests/test_transform.py

+                # TODO: add more test cases once DM-9826 resolved
+
+    def checkGetInverseWithMap(self, transform, nIn, msg):
+        """Test Transform<fromName>To<toName>.getInverse with a specific mapping.


What do you mean by "with a specific mapping"? A transform can be constructed with a mapping or a frame set, but the results is that it contains a frame set in either case.

To my mind the name suggests that the user will provide a mapping as an argument to this method (I am not suggesting you do that; the current test seems excellent).

Can you call this checkGetInverse and skip the WithMap?

I need to give it a distinct name from the method immediately above... checkGetInverseImpl?

If I am reading the code correctly:

checkGetInverse creates all possible transforms for a given pair of endpoint names, creating each frame set or mapping as it sees fit.

checkGetInverseWithMap checks one transform.

I'm asking for a less confusing name scheme. To be fair I'll propose one, but do as you like:

Rename checkGetInverseWithMap to checkGetInverse and ditch old version of the latter.

Make checkTransform call checkGetInverseWithMap with each valid transform that it constructs.

Or if that's too much work for checkTransform you could make a new method testGetInverse and have it build all valid transforms that you want to test, then call checkTransform from there.

I wonder if I should break checkTransform up a bit, e.g. I could imagine a function by that name which takes a valid transform as an argument and checks basics. Then testTransform would have to construct the transforms. I'm not sure where I would move the code that tries to construct invalid transforms -- perhaps a new testX method. What do you think?

r-owen

Looks great. I had a few minor suggestions.

r-owen · 2017-03-24T19:09:24Z

src/geom/Transform.cc

+Transform<ToEndpoint, FromEndpoint> Transform<FromEndpoint, ToEndpoint>::getInverse() const {
+    auto inverse = std::dynamic_pointer_cast<ast::FrameSet>(_frameSet->getInverse());
+    if (!inverse) {
+        // std::bad_cast more appropriate, but doesn't aid in debugging


I like the code but find the note confusing. This situation should never occur, so logic_error does seem the best choice.

r-owen · 2017-03-24T19:11:15Z

python/lsst/afw/geom/endpoint.cc

        auto frame = self.makeFrame();
        return frame->copy();
    });
 }

+// Endpoint comparison useful in Python but not C++


I'm not convinced by this assertion. If it would not be a lot more work I recommend making this available to C++.

r-owen · 2017-03-24T19:15:36Z

tests/test_endpoint.py

+        """
+        for i1, point1 in enumerate(self.makeEndpoints()):
+            for i2, point2 in enumerate(self.makeEndpoints()):
+                self.assertEqual(i1 == i2, point1 == point2)


Cute! But the message in case of an error will not be clear. I suggest formatting a useful error message to be passed as the msg argument to self.assertEqual. (This might be easier if you handled the i1 == i2 case separately from the other case, but I'll leave that to you).

Also, please test != as well as ==.

r-owen · 2017-03-24T19:19:44Z

tests/test_transform.py

@@ -400,11 +513,107 @@ def checkTransformFromFrameSet(self, fromName, toName):

                    self.checkTransformation(permTransform, mapping=polyMap, msg=msg)

+    def checkAllGetInverse(self, fromName, toName):


A nicely written method, but what does the "All" in the name tell me? Why not "checkGetInverse"?

Several methods now use these methods to provide more predictable exception behavior.

This change is primarily intended for unit tests that need to verify which Endpoint is being used by a particular object.

kfindeisen requested a review from r-owen March 17, 2017 02:37

jdswinbank reviewed Mar 17, 2017

View reviewed changes

r-owen approved these changes Mar 17, 2017

View reviewed changes

kfindeisen force-pushed the tickets/DM-9595 branch 5 times, most recently from a6f13e5 to 022a7b6 Compare March 24, 2017 17:59

r-owen approved these changes Mar 24, 2017

View reviewed changes

kfindeisen added 4 commits March 27, 2017 11:26

Implement Transform.hasForward and hasInverse.

554e9e5

Several methods now use these methods to provide more predictable exception behavior.

Add python-only equality test for Endpoints.

398f068

This change is primarily intended for unit tests that need to verify which Endpoint is being used by a particular object.

Add Transform::getInverse.

8efb11e

Style cleanup of Transform.

d093cb4

kfindeisen force-pushed the tickets/DM-9595 branch from 022a7b6 to d093cb4 Compare March 27, 2017 18:26

kfindeisen merged commit d093cb4 into master Mar 27, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-9595: Allow Transform to return its inverse #192

DM-9595: Allow Transform to return its inverse #192

kfindeisen commented Mar 17, 2017

jdswinbank Mar 17, 2017

kfindeisen Mar 17, 2017

r-owen Mar 17, 2017

kfindeisen Mar 17, 2017

r-owen Mar 17, 2017

kfindeisen Mar 17, 2017

r-owen left a comment

r-owen Mar 17, 2017

kfindeisen Mar 17, 2017

r-owen Mar 17, 2017

r-owen Mar 17, 2017

kfindeisen Mar 17, 2017

r-owen Mar 17, 2017

r-owen Mar 17, 2017

r-owen Mar 17, 2017

kfindeisen Mar 17, 2017 •

edited

r-owen Mar 17, 2017 •

edited

r-owen Mar 17, 2017

kfindeisen Mar 17, 2017 •

edited

r-owen Mar 17, 2017 •

edited

r-owen left a comment

r-owen Mar 24, 2017

r-owen Mar 24, 2017

r-owen Mar 24, 2017

r-owen Mar 24, 2017

		@@ -400,11 +513,107 @@ def checkTransformFromFrameSet(self, fromName, toName):

		self.checkTransformation(permTransform, mapping=polyMap, msg=msg)

		def checkAllGetInverse(self, fromName, toName):

DM-9595: Allow Transform to return its inverse #192

DM-9595: Allow Transform to return its inverse #192

Conversation

kfindeisen commented Mar 17, 2017

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

Choose a reason for hiding this comment

kfindeisen Mar 17, 2017 • edited

Choose a reason for hiding this comment

r-owen Mar 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Mar 17, 2017 • edited

Choose a reason for hiding this comment

r-owen Mar 17, 2017 • edited

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 Mar 17, 2017 •

edited

r-owen Mar 17, 2017 •

edited

kfindeisen Mar 17, 2017 •

edited

r-owen Mar 17, 2017 •

edited