DM-9895 FrameSet frames not preserved by Transform(frameSet) constructor #198

r-owen · 2017-03-22T00:05:52Z

No description provided.

kfindeisen

I spotted a few inconsistencies and I'm a bit concerned by the code duplication in test_transform, but no major issues.

kfindeisen · 2017-03-22T00:21:43Z

src/geom/Endpoint.cc

    if (!skyFramePtr) {
        std::ostringstream os;
-        os << "frame is a \"" << framePtr->getClass() << "\", not a SkyFrame";
+        os << "frame is a " << getFrame(framePtr)->getClass() << ", not a SkyFrame";


Are you sure you want PointEndpoint::normalizeFrame to report the original class and SpherePointEndpoint::normalizeFrame to report FrameSets as Frames? I don't see the reason for the inconsistency.

Thanks for catching this. I'll fix PointEndpoint<N>::normalizeFrame to show the frame class (not FrameSet)

kfindeisen · 2017-03-22T00:28:25Z

tests/test_transform.py

@@ -37,6 +38,122 @@ def makeRawArrayData(nPoints, nAxes, delta=0.123):
 def makeRawPointData(nAxes, delta=0.123):
    return [i*delta for i in range(nAxes)]

+
+def makeEndpoint(name, nAxes):


Handy, thanks!

kfindeisen · 2017-03-22T00:30:27Z

tests/test_transform.py

+
+
+def makeGoodFrame(name, nAxes):
+    """Return the appropriate frame for the given name and nAzes


nAzes -> nAxes

kfindeisen · 2017-03-22T00:33:38Z

tests/test_transform.py

+    @param[in] nAxes  number of input axes
+    """
+    if name == "Generic":
+        return ()


Should this be the empty list rather than the empty tuple?

kfindeisen · 2017-03-22T00:36:24Z

tests/test_transform.py

+
+
+def makeBadFrames(name, nAxes):
+    """Return a list of 0 or more frames that are not a valid match for name and nAxes


nAxes is unused. By "not a valid match for nAxes", do you mean, e.g., returning a 3D frame of the correct type when nAxes == 2?

Thank you for realizing nAxes was irrelevant. The point of this subroutine is to construct a collection of 0 or more astshim frames that are not compatible with the named endpoint. Since all frames are compatible with GenericEndpoint and the number of axes is known for all other endpoints, nAxes is indeed irrelevant.

kfindeisen · 2017-03-22T17:38:40Z

tests/test_transform.py

+                inPoint = fromEndpoint.pointFromData(rawInPoint)
+                outPoint = transform.tranForward(inPoint)
+                rawOutPoint = toEndpoint.dataFromPoint(outPoint)
+                # TODO replace all use of np.allclose with assertFloatsAlmostEqual once DM-9707 is fixed


Copy-and-pasted comment.

Can you factor out the common code here and in checkTransformFromMapping into a method that takes a Transform?

kfindeisen · 2017-03-22T17:41:19Z

tests/test_transform.py

+
+                # If frameSet has a SkyFrame as base or current, try permuting that (in place);
+                # the Transform constructor should undo the permutation.
+                # Warning: you must permute the axes of the SkYFrame once it is in the frameSet,


Capitalization error in SkYFrame

kfindeisen · 2017-03-22T17:46:50Z

tests/test_transform.py

+                # rather than before adding it to the FrameSet, else the behavior is wrong
+                for permFrameSet, isBasePermuted, isCurrPermuted in permFrameSetIter(frameSet):
+                    if isBasePermuted:
+                        self.assertEqual(permFrameSet.getFrame(astshim.FrameSet.BASE).getLonAxis(), 2)


Do you need an else case for these setup assertions?

I think the code is right. If isBasePermuted then the code permutes the base frame (which is a bit messy because of the need to temporarily set it to be the current frame). If isCurrPermuted then the code permutes the current frame (which is trivial). There is no interaction between these flags, though at least one of them will be true.

Am I missing something? Can you say more about what you had in mind?

I assume the purpose of this block is to double-check the output of permFrameSetIter before using it to actually test FrameSet or Transform.

I was thinking that if you want to check getLonAxis() == 2 when isBasePermuted, then you would also want to check that getLonAxis() == 1 when !isBasePermuted.

It's a bit of a nuisance, since if isXPermuted is false then the axis may not be a SkyFrame, but I see your point. I'll do it.

Actually, I completely missed the part about getLonAxis() not being defined for all frames... that does make my suggestion rather pointless.

kfindeisen · 2017-03-22T17:47:20Z

tests/test_transform.py

+                    if isCurrPermuted:
+                        self.assertEqual(permFrameSet.getFrame(astshim.FrameSet.CURRENT).getLonAxis(), 2)
+                    transformBasePerm = transformClass(permFrameSet)
+                    # the base frame set in the transform should NOT be permuted


I don't understand this comment. It looks below like you're requiring that neither the base nor the current frame be permuted.

What I am testing is that SpherePoint.normalizeFrame correctly unpermutes the axes into the standard order. I changed the comment as follows in hopes of clarifying this:

# Verify that the base and current frames in the frame set in the *transform* # are *not* permuted (i.e. that SpherePointEndpoint.normalizeFrame correctly # set the axes to the standard order long, lat)

kfindeisen · 2017-03-22T17:48:11Z

tests/test_transform.py

+                    fromEndpoint = transform.getFromEndpoint()
+
+                    # the transform should work identically to `transform`
+                    nPoints = 7  # arbitrary


Again, can this duplicate code be factored out?

Yes. Done as part of the earlier refactoring. I'm really glad you suggested that. It was a huge help.

Transform(FrameSet) was never called in Python, because Transform(Mapping) was listed first in the pybind11 wrapper, and so was always called. Fixing this turned up several bugs in normalizing frames: - naive tests in PointEndpoint and SpherePointEndpoint were fooled when a FrameSet was passed in as a Frame. - SpherePointEndpoint needed a tweak so that permAxes was called on the frame argument, rather than a copy when the frame was a FrameSet. I addded tests for all these issues to tests/test_transform.py

Replace `self.assertTrue(numpy.allclose(...))` with `numpy.testing.assert_allclose(...)` in the two tests where I found it.

kfindeisen approved these changes Mar 22, 2017

View reviewed changes

r-owen force-pushed the tickets/DM-9895 branch 3 times, most recently from c977d3c to 4e9cc51 Compare March 22, 2017 23:34

r-owen added 3 commits March 22, 2017 17:31

Use assert_allclose in remaining tests

82a450c

Replace `self.assertTrue(numpy.allclose(...))` with `numpy.testing.assert_allclose(...)` in the two tests where I found it.

Fix flake8 warnings in testKernel.py

e0d5188

r-owen force-pushed the tickets/DM-9895 branch from 4e9cc51 to e0d5188 Compare March 23, 2017 00:31

r-owen merged commit e0d5188 into master Mar 23, 2017

ktlim deleted the tickets/DM-9895 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-9895 FrameSet frames not preserved by Transform(frameSet) constructor #198

DM-9895 FrameSet frames not preserved by Transform(frameSet) constructor #198

r-owen commented Mar 22, 2017

kfindeisen left a comment

kfindeisen Mar 22, 2017

r-owen Mar 22, 2017

kfindeisen Mar 22, 2017

kfindeisen Mar 22, 2017

kfindeisen Mar 22, 2017

kfindeisen Mar 22, 2017

r-owen Mar 22, 2017

kfindeisen Mar 22, 2017

kfindeisen Mar 22, 2017

kfindeisen Mar 22, 2017

r-owen Mar 22, 2017

kfindeisen Mar 22, 2017

r-owen Mar 22, 2017

kfindeisen Mar 23, 2017

kfindeisen Mar 22, 2017

r-owen Mar 22, 2017

kfindeisen Mar 22, 2017

r-owen Mar 22, 2017



		def makeGoodFrame(name, nAxes):
		"""Return the appropriate frame for the given name and nAzes



		def makeBadFrames(name, nAxes):
		"""Return a list of 0 or more frames that are not a valid match for name and nAxes

DM-9895 FrameSet frames not preserved by Transform(frameSet) constructor #198

DM-9895 FrameSet frames not preserved by Transform(frameSet) constructor #198

Conversation

r-owen commented Mar 22, 2017

kfindeisen 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

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