DM-12102: add input validation for SpherePoint(double, double, AngleUnits) constructor #283

r-owen · 2017-10-04T18:07:04Z

No description provided.

kfindeisen

Looks better, but the test coverage of testDoubleDoubleUnitsConstructor is still relatively sparse. Can you at least check that getLongitude gives an in-range value for weird constructor arguments?

kfindeisen · 2017-10-04T18:08:53Z

tests/test_spherePoint.py

+        with self.assertRaises(pexEx.InvalidParameterError):
+            SpherePoint(-42.0, 91.0, degrees)  # latitude out of range
+        with self.assertRaises(pexEx.InvalidParameterError):
+            SpherePoint(-42.0, -91.0, degrees)  # latitude out of range


Could you also add a test that e.g. negative longitudes get wrapped correctly? (Possibly as part of testGetLongitudeValue rather than here, since that's where the tests are done for the original constructor).

I don't know if you want the other tests from testInit2ArgErrors, like infinite latitude.

r-owen · 2017-10-04T19:01:13Z

I greatly expanded the testing of the new SpherePoint(double, double, AngleUnit) constructor and integrated it into testing of the SpherePoint(Angle, Angle) constructor. I also took the liberty of switching to the new constructor in some places where it made the test easier to read. Please have another look.

kfindeisen

Looks good. Some code cleanup suggestions. Only thing that really bothers me is the long nested check... methods, but maybe those look more readable to a Python programmer.

kfindeisen · 2017-10-04T19:09:21Z

tests/test_spherePoint.py

        """
        # Latitude should be checked for out-of-range.
        for lat in self._poleLatitudes:
            with self.assertRaises(pexEx.InvalidParameterError):
                SpherePoint(0.0*degrees, self.nextUp(lat))
            with self.assertRaises(pexEx.InvalidParameterError):
+                SpherePoint(0.0, self.nextUp(lat).asDegrees(), degrees)


Might be cleaner to call np.nextafter directly, or modify nextUp and nextDown to work on floating-point numbers.

The current code is a bit clumsy but is obviously the same test, which I think is worth the clumsiness. I like the fact that nextUp works on Angles and am reluctant to change that.

I meant "in addition to Angles", but ok.

kfindeisen · 2017-10-04T19:14:39Z

tests/test_spherePoint.py

@@ -155,8 +154,7 @@ def testInitNArgFail(self):
    def testGetLongitudeValue(self):
        """Test if getLongitude() returns the expected value.
        """
-        for lon, lat in self._dataset:
-            point = SpherePoint(lon, lat)
+        def checkGetLongitudeValue(point, lon, lat):


I think this is a bit too long to treat as a lambda. Consider making a private member of SpherePointTestSuite for readability.

Technically that this is not a "lambda", since Python uses the keyword lambda for those. But I understand and will make them methods.

I reverted all the new in-line functions and am using better iteration instead. This really cleans up the code.

kfindeisen · 2017-10-04T19:18:12Z

tests/test_spherePoint.py

@@ -191,8 +193,7 @@ def testTicket1394(self):
    def testGetLatitudeValue(self):
        """Test if getLatitude() returns the expected value.
        """
-        for lon, lat in self._dataset:
-            point = SpherePoint(lon, lat)
+        def checkGetLatitudeValue(point, lon, lat):


Again, having this in checkGetLatitudeValue breaks up the flow of the code.

kfindeisen · 2017-10-04T19:21:33Z

tests/test_spherePoint.py

+            self.assertAlmostEqual(newLat.asDegrees(), lat.asDegrees())
+
+        for lonLatDeg, vector in pointList:
+            # Convert to Point3D.


Does this comment belong inside checkGetVector?

No idea. It's gone now.

kfindeisen · 2017-10-04T19:21:43Z

tests/test_spherePoint.py

-        for lonLat, vector in pointList:
-            # Convert to Point3D.
-            point = SpherePoint(lonLat[0]*degrees, lonLat[1]*degrees)
+        def checkGetVector(point, lon, lat, vector):


Long lambda.

kfindeisen · 2017-10-04T19:24:22Z

tests/test_spherePoint.py

+            [SpherePoint(42.0*degrees, lat) for lat in self._poleLatitudes] + \
+            [SpherePoint(42.0, lat.asDegrees(), degrees) for lat in self._poleLatitudes] + \
+            [SpherePoint(42.0*degrees, 0.0*radians - lat) for lat in self._poleLatitudes] + \
+            [SpherePoint(42.0, (0.0*radians - lat).asDegrees(), degrees) for lat in self._poleLatitudes] + \


-lat.asDegrees() would be more concise.

Actually, since I added a negation operator to Angle, all the references to 0.0*radians can probably be removed.

kfindeisen · 2017-10-04T19:27:26Z

tests/test_spherePoint.py

@@ -333,8 +360,7 @@ def testGetItemError(self):
    def testGetItemValue(self):
        """Test if indexing returns the expected value.
        """
-        for lon, lat in self._dataset:
-            point = SpherePoint(lon, lat)
+        def checkGetItemValue(point, lon, lat):


Long lambda.

kfindeisen · 2017-10-04T20:33:36Z

tests/test_spherePoint.py

+                for oldElement, newElement in zip(vector, newVector):
+                    self.assertAlmostEqual(oldElement, newElement)
+
+                # Convert back to spherical.


If you're removing the "Convert to Point3D" comment, you might remove this one as well

Sure. I thought I had reverted the other comment to its original place but maybe not. I'll remove both of them as I don't think they add much.

kfindeisen

Looks good, there's just a few places where you duplicate the try-all-constructors logic from pointSet.

kfindeisen · 2017-10-04T20:34:16Z

tests/test_spherePoint.py

+                SpherePoint(lon, lat),
+                SpherePoint(lon.asDegrees(), lat.asDegrees(), degrees),
+                SpherePoint(lon.asRadians(), lat.asRadians(), radians),
+            ):


Very thorough!

kfindeisen · 2017-10-04T20:35:58Z

tests/test_spherePoint.py

+                SpherePoint(lon, lat),
+                SpherePoint(lon.asDegrees(), lat.asDegrees(), degrees),
+                SpherePoint(lon.asRadians(), lat.asRadians(), radians),
+            ):


Why not just use self.pointSet?

I do this in two places because in those places the test needs the input longitude or latitude and pointSet does not provide them. I considered a property pointLonLatSet but I felt it was excessive clutter for just two tests.

Good point. A comment explaining that might be helpful, though.

kfindeisen · 2017-10-04T20:36:35Z

tests/test_spherePoint.py

+                SpherePoint(lon, lat),
+                SpherePoint(lon.asDegrees(), lat.asDegrees(), degrees),
+                SpherePoint(lon.asRadians(), lat.asRadians(), radians),
+            ):


Again, use self.pointSet instead of a double loop.

Check longitude in new SpherePoint(double, double, AngleUnit) constructor.

and use that constructor where it makes more sense in existing tests

kfindeisen approved these changes Oct 4, 2017

View reviewed changes

r-owen force-pushed the tickets/DM-12102 branch from dec5819 to 1ec2325 Compare October 4, 2017 20:32

kfindeisen reviewed Oct 4, 2017

View reviewed changes

kfindeisen approved these changes Oct 4, 2017

View reviewed changes

r-owen force-pushed the master branch from 6058802 to 29f79c4 Compare October 4, 2017 20:52

r-owen added 3 commits October 4, 2017 13:53

Add SpherePoint(double, double, AngleUnit) ctor

2bf2040

Check longitude in new SpherePoint constructor

f069742

Check longitude in new SpherePoint(double, double, AngleUnit) constructor.

Expand unit test for SpherePoint(lon, lat, units)

112e94a

and use that constructor where it makes more sense in existing tests

r-owen force-pushed the tickets/DM-12102 branch from 1ec2325 to 112e94a Compare October 4, 2017 20:54

r-owen merged commit 112e94a into master Oct 4, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-12102: add input validation for SpherePoint(double, double, AngleUnits) constructor #283

DM-12102: add input validation for SpherePoint(double, double, AngleUnits) constructor #283

r-owen commented Oct 4, 2017

kfindeisen left a comment

kfindeisen Oct 4, 2017

kfindeisen Oct 4, 2017

r-owen commented Oct 4, 2017

kfindeisen left a comment

kfindeisen Oct 4, 2017

r-owen Oct 4, 2017

kfindeisen Oct 4, 2017

kfindeisen Oct 4, 2017

r-owen Oct 4, 2017

r-owen Oct 4, 2017

kfindeisen Oct 4, 2017

kfindeisen Oct 4, 2017

r-owen Oct 4, 2017

kfindeisen Oct 4, 2017

kfindeisen Oct 4, 2017

kfindeisen Oct 4, 2017

kfindeisen Oct 4, 2017

r-owen Oct 4, 2017

kfindeisen left a comment

kfindeisen Oct 4, 2017

kfindeisen Oct 4, 2017

r-owen Oct 4, 2017

kfindeisen Oct 4, 2017

kfindeisen Oct 4, 2017

DM-12102: add input validation for SpherePoint(double, double, AngleUnits) constructor #283

DM-12102: add input validation for SpherePoint(double, double, AngleUnits) constructor #283

Conversation

r-owen commented Oct 4, 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

r-owen commented Oct 4, 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

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