From f335e70196392bb83d8a0bfcc8775beb47f0018b Mon Sep 17 00:00:00 2001
From: dellaert <dellaert@cc.gatech.edu>
Date: Sun, 24 May 2015 10:25:19 -0700
Subject: [PATCH] Fixed some issues and further testing

---
 gtsam/base/Group.h                  | 10 +++++++---
 gtsam/geometry/Cyclic.h             |  7 +++----
 gtsam/geometry/tests/testCyclic.cpp |  8 ++++----
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/gtsam/base/Group.h b/gtsam/base/Group.h
index a9abf968d..f56ba9685 100644
--- a/gtsam/base/Group.h
+++ b/gtsam/base/Group.h
@@ -88,13 +88,15 @@ check_group_invariants(const G& a, const G& b, double tol = 1e-9) {
 namespace internal {
 
 /// A helper class that implements the traits interface for groups.
+/// Assumes that constructor yields identity
 template<class Class>
 struct GroupTraits {
   typedef group_tag structure_category;
-  static Class Identity() { return Class::Identity(); }
+  static Class Identity() { return Class(); }
 };
 
 /// A helper class that implements the traits interface for additive groups.
+/// Assumes existence of three additive operators
 template<class Class>
 struct AdditiveGroupTraits : GroupTraits<Class> {
     typedef additive_group_tag group_flavor; \
@@ -104,9 +106,10 @@ struct AdditiveGroupTraits : GroupTraits<Class> {
 };
 
 /// A helper class that implements the traits interface for multiplicative groups.
+/// Assumes existence of operators * and /, as well as inverse method
 template<class Class>
 struct MultiplicativeGroupTraits : GroupTraits<Class> {
-    typedef additive_group_tag group_flavor; \
+    typedef multiplicative_group_tag group_flavor; \
     static Class Compose(const Class &g, const Class & h) { return g * h;} \
     static Class Between(const Class &g, const Class & h) { return g.inverse() * h;} \
     static Class Inverse(const Class &g) { return g.inverse();}
@@ -114,6 +117,7 @@ struct MultiplicativeGroupTraits : GroupTraits<Class> {
 }  // namespace internal
 
 /// Template to construct the direct sum of two additive groups
+/// Assumes existence of three additive operators for both groups
 template<typename G, typename H>
 class DirectSum: public std::pair<G, H> {
   BOOST_CONCEPT_ASSERT((IsGroup<G>));  // TODO(frank): check additive
@@ -127,7 +131,7 @@ public:
   DirectSum(const G& g, const H& h):std::pair<G,H>(g,h) {
   }
   /// Default constructor yields identity
-  DirectSum():std::pair<G,H>(G::Identity(),H::Identity()) {
+  DirectSum():std::pair<G,H>(traits<G>::Identity(),traits<H>::Identity()) {
   }
   static DirectSum Identity() {
     return DirectSum();
diff --git a/gtsam/geometry/Cyclic.h b/gtsam/geometry/Cyclic.h
index 8aa205aa3..88a04ab2d 100644
--- a/gtsam/geometry/Cyclic.h
+++ b/gtsam/geometry/Cyclic.h
@@ -31,9 +31,8 @@ public:
       i_(i) {
     assert(i < N);
   }
-  /// Identity element
-  static Cyclic Identity() {
-    return Cyclic(0);
+  /// Default constructor yields identity
+  Cyclic():i_(0) {
   }
   /// Cast to size_t
   operator size_t() const {
@@ -61,7 +60,7 @@ public:
   }
 };
 
-/// Define cyclic group traits to be a model of the Additive Group concept
+/// Define cyclic group to be a model of the Additive Group concept
 template<size_t N>
 struct traits<Cyclic<N> > : internal::AdditiveGroupTraits<Cyclic<N> >, //
     Testable<Cyclic<N> > {
diff --git a/gtsam/geometry/tests/testCyclic.cpp b/gtsam/geometry/tests/testCyclic.cpp
index 22b4dc209..20404a14f 100644
--- a/gtsam/geometry/tests/testCyclic.cpp
+++ b/gtsam/geometry/tests/testCyclic.cpp
@@ -81,7 +81,7 @@ TEST(Cyclic, Negation2) {
 //******************************************************************************
 TEST(Cyclic , Invariants) {
   G g(2), h(1);
- check_group_invariants(g,h);
+  EXPECT(check_group_invariants(g,h));
 }
 
 //******************************************************************************
@@ -128,9 +128,9 @@ TEST(Cyclic , DirectSum) {
   EXPECT(assert_equal(c, a - b));
   EXPECT(assert_equal(a, b - c));
   EXPECT(assert_equal(b, c - a));
-  check_group_invariants(a, b);
-  check_group_invariants(b, c);
-  check_group_invariants(c, a);
+  EXPECT(check_group_invariants(a, b));
+  EXPECT(check_group_invariants(b, c));
+  EXPECT(check_group_invariants(c, a));
 }
 
 //******************************************************************************