DM-7913: Implement RFC-240: make Angle's named methods const... #184

kfindeisen · 2017-03-01T21:12:46Z

In addition to the changes requested in DM-7913, this change includes some code cleanup of Angle... which I might have gotten a bit carried away with:

clang-format pass
splitting declaration/definition of complex methods in Angle
added unary - operator (surprising omission in previous API, came up when writing tests for separation)
added support for C++11 features
reformatted and greatly expanded documentation

r-owen

Looks great! A few minor suggestions including removing outdated SWIG macros and comments and fixing a possible edge case bug.

r-owen · 2017-03-09T19:36:35Z

include/lsst/afw/geom/Angle.h

+inline constexpr double radToArcsec(double x) noexcept { return x * 3600. * 180. / PI; }
+inline constexpr double radToMas(double x) noexcept { return x * 1000. * 3600. * 180. / PI; }
+inline constexpr double arcsecToRad(double x) noexcept { return (x / 3600.) * PI / 180.; }
+inline constexpr double masToRad(double x) noexcept { return (x / (1000. * 3600.)) * PI / 180.; }

 // NOTE, if you add things here, you must also add them to
 //    python/lsst/afw/geom/__init__.py
 // (if you want them to accessible from python)


I suggest deleting this code comment; it is outdated and not very useful

r-owen · 2017-03-09T19:37:49Z

include/lsst/afw/geom/Angle.h

 */
-class AngleUnit {
+class AngleUnit
+#ifndef SWIG


Please delete all SWIG macro commands and all comments that include SWIG. Note that some SWIG comments use "swig" (lowercase).

r-owen · 2017-03-09T19:49:22Z

include/lsst/afw/geom/Angle.h

 /************************************************************************************************************/

 class Angle;
 /**
- * \brief A class used to convert scalar POD types such as double to Angle
+ * A class used to convert scalar POD types such as `double` to Angle.
 *
 * For example:


Could you add a bit more information to make it clear that degrees is an instance of AngleUnit?

r-owen · 2017-03-09T19:50:59Z

include/lsst/afw/geom/Angle.h

-inline bool lsst::afw::geom::AngleUnit::operator==(lsst::afw::geom::AngleUnit const &rhs) const {
-	return (_val == rhs._val);
+inline constexpr bool AngleUnit::operator==(AngleUnit const& rhs) const noexcept {
+    return (_val == rhs._val);
 }

 // NOTE, if you add things here, remember to also add them to
 //    python/lsst/afw/geom/__init__.py

 // swig likes this way of initialising the constant, so don't mess with it;
 // N.b. swig 1.3 doesn't like PI/(60*180)


(SWIG comment to delete; I won't point out any more)

r-owen · 2017-03-09T19:53:02Z

include/lsst/afw/geom/Angle.h

-inline bool lsst::afw::geom::AngleUnit::operator==(lsst::afw::geom::AngleUnit const &rhs) const {
-	return (_val == rhs._val);
+inline constexpr bool AngleUnit::operator==(AngleUnit const& rhs) const noexcept {
+    return (_val == rhs._val);
 }

 // NOTE, if you add things here, remember to also add them to
 //    python/lsst/afw/geom/__init__.py


Please delete (or fix) this comment.

r-owen · 2017-03-09T20:05:53Z

include/lsst/afw/geom/Angle.h

@@ -246,73 +299,182 @@ Angle const NullAngle = Angle(-1000000., degrees);
 * N.b. We need both int and double versions to avoid ambiguous overloading due to implicit conversion of
 * Angle to double


ANGLE_OP does not appear to do anything with int; I suspect this comment refers to ANGLE_OP_TYPE; if so please move it.

r-owen · 2017-03-09T20:14:11Z

include/lsst/afw/geom/Angle.h

+template <typename T>
+inline constexpr Angle operator*(T lhs, AngleUnit rhs) noexcept {
+    static_assert(std::is_arithmetic<T>::value,
+                  "Only numeric types may be converted to Angles using degrees/radians!");


This error message could be improved. Perhaps something like "Only numeric types may multiplied by an AngleUnit such as degrees or radians to create an Angle".

r-owen · 2017-03-09T20:15:41Z

include/lsst/afw/geom/Angle.h

+
+inline double Angle::toUnitSphereDistanceSquared() const noexcept {
+    return 2. * (1. - std::cos(asRadians()));
+    // == 4.0 * pow(std::sin(0.5 * asRadians()), 2.0)


Is one preferred over the other in a particular regime? If so, is there any point to a conditional?

In any case it might be worth a brief comment as to why one was chosen and the other not

There might be, but this is preexisting code, and the initial commit (2b49698) doesn't provide any clues.

Hmm... both methods are used only in afw/table/Match.cc. Perhaps they could at least be moved there, since they aren't really operations on angles in the general sense.

r-owen · 2017-03-09T20:20:40Z

include/lsst/afw/geom/Angle.h

+    }
+    // maximum relative roundoff error for subtraction is 2 epsilon
+    if (wrapped - refAngRad < -PI) {
+        wrapped -= wrapped * 2.0 * std::numeric_limits<double>::epsilon();


Shouldn't this be +=?

I think this is equivalent to wrapped *= (1.0 - 2ε), regardless of whether wrapped is positive or negative. So neither += nor -= really make sense (e.g., consider if wrapped = 0 and refAngRad = π + ε).

Try as I might, I can't come up with a test case that exposes any problems with the code as-is (although removing this block does cause existing tests to fail). I'm not comfortable trying to fix this if I can't see the effects, so at most I can file DM-9765 to investigate later... 😞

r-owen · 2017-03-09T20:29:24Z

python/lsst/afw/geom/angle/angle.cc

+    // Without an explicit wrapper, Python lets Angle / Angle -> Angle
+    clsAngle.def("__truediv__", [](Angle const& self, Angle const& other) {
+        throw py::type_error("unsupported operand type(s) for /: 'Angle' and 'Angle'");
+    });

    clsAngle.def("__float__", &Angle::operator double);


Can we get rid of this? I suppose it may require an RFC (though if it passed Jenkins...)

This allows method bodies to use the full Angle API, particularly operators. Trivial getters have not been moved to minimize code duplication.

Client code is generally simpler if wrap methods return an altered Angle instead of modifying the object they are called on.

This change allows Angles to be used in compile-time expressions and take advantage of noexcept optimizations. It also simplifies some existing metaprogramming using C++11-only tools.

This commit ensures all API elements are documented, and that documentation complies with current style guidelines.

These methods were added to Angle as part of 2b49698 to be used in table::Match. They have not been used anywhere else, their use case is more specific than other Angle operations, and they do not need any access to Angle's internals (cf. Effective C++ 3rd. Ed., Item 23). In addition, making them an implementation detail of Match reduces coupling between afw::geom and afw::table, which may be useful in the long run.

kfindeisen requested review from laurenam and removed request for laurenam March 1, 2017 21:29

kfindeisen force-pushed the tickets/DM-7913 branch 4 times, most recently from 270471c to a38f3ae Compare March 9, 2017 18:23

r-owen approved these changes Mar 9, 2017

View reviewed changes

kfindeisen added 8 commits March 9, 2017 15:23

Format/style cleanup of Angle.

20b8a2a

Move inline method definitions to end of Angle.h.

2e6fa18

This allows method bodies to use the full Angle API, particularly operators. Trivial getters have not been moved to minimize code duplication.

Give Angle.wrap*() a functional API.

e4ab28f

Client code is generally simpler if wrap methods return an altered Angle instead of modifying the object they are called on.

Implement Angle::separation.

9290528

Add negation operator to Angle.

9ffedb2

Remove NullAngle.

8590cc7

C++11 update pass for Angle.

5bfd67f

This change allows Angles to be used in compile-time expressions and take advantage of noexcept optimizations. It also simplifies some existing metaprogramming using C++11-only tools.

Documentation cleanup pass for Angle.

07b031b

This commit ensures all API elements are documented, and that documentation complies with current style guidelines.

kfindeisen force-pushed the tickets/DM-7913 branch from a38f3ae to 07b031b Compare March 10, 2017 00:44

kfindeisen merged commit 7f9d117 into master Mar 11, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-7913: Implement RFC-240: make Angle's named methods const... #184

DM-7913: Implement RFC-240: make Angle's named methods const... #184

kfindeisen commented Mar 1, 2017 •

edited

r-owen left a comment

r-owen Mar 9, 2017

r-owen Mar 9, 2017

r-owen Mar 9, 2017

r-owen Mar 9, 2017

r-owen Mar 9, 2017

r-owen Mar 9, 2017

r-owen Mar 9, 2017

r-owen Mar 9, 2017

kfindeisen Mar 9, 2017

kfindeisen Mar 9, 2017

r-owen Mar 9, 2017

kfindeisen Mar 10, 2017 •

edited

kfindeisen Mar 10, 2017 •

edited

r-owen Mar 9, 2017

		@@ -246,73 +299,182 @@ Angle const NullAngle = Angle(-1000000., degrees);
		* N.b. We need both int and double versions to avoid ambiguous overloading due to implicit conversion of
		* Angle to double

DM-7913: Implement RFC-240: make Angle's named methods const... #184

DM-7913: Implement RFC-240: make Angle's named methods const... #184

Conversation

kfindeisen commented Mar 1, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Mar 10, 2017 • edited

Choose a reason for hiding this comment

kfindeisen Mar 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen commented Mar 1, 2017 •

edited

kfindeisen Mar 10, 2017 •

edited

kfindeisen Mar 10, 2017 •

edited