Use explicit characters and add determinant error checking

sethrj · Jan 20, 2024 · 0f913f7 · 0f913f7
1 parent 0b23d87
commit 0f913f7
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 23 deletions.
diff --git a/src/orange/transform/SignedPermutation.cc b/src/orange/transform/SignedPermutation.cc
@@ -7,6 +7,8 @@
 //---------------------------------------------------------------------------//
 #include "SignedPermutation.hh"
 
+#include "orange/MatrixUtils.hh"
+
 namespace celeritas
 {
 //---------------------------------------------------------------------------//
@@ -15,7 +17,7 @@ namespace celeritas
  */
 SignedPermutation::SignedPermutation()
     : SignedPermutation{
-        SignedAxes{{{psign, Axis::x}, {psign, Axis::y}, {psign, Axis::z}}}}
+        SignedAxes{{{'+', Axis::x}, {'+', Axis::y}, {'+', Axis::z}}}}
 {
 }
 
@@ -27,12 +29,14 @@ SignedPermutation::SignedPermutation(SignedAxes permutation) : compressed_{0}
 {
     EnumArray<Axis, bool> encountered_ax{false, false, false};
 
+    SquareMatrix<int, 3> explicit_mat;
+
     for (auto ax : {Axis::z, Axis::y, Axis::x})
     {
         auto new_ax_sign = permutation[ax];
-        CELER_VALIDATE(
-            new_ax_sign.first == psign || new_ax_sign.first == msign,
-            << "invalid permutation sign '" << new_ax_sign.first << "'");
+        CELER_VALIDATE(new_ax_sign.first == '+' || new_ax_sign.first == '-',
+                       << "invalid permutation sign '" << new_ax_sign.first
+                       << "'");
         CELER_VALIDATE(new_ax_sign.second < Axis::size_,
                        << "invalid permutation axis");
         CELER_VALIDATE(!encountered_ax[new_ax_sign.second],
@@ -42,11 +46,27 @@ SignedPermutation::SignedPermutation(SignedAxes permutation) : compressed_{0}
 
         // Push back "flip bit"
         compressed_ <<= 1;
-        compressed_ |= (new_ax_sign.first == msign ? 0b1 : 0b0);
+        compressed_ |= (new_ax_sign.first == '-' ? 0b1 : 0b0);
         // Push back "new axis"
         compressed_ <<= 2;
         compressed_ |= to_int(new_ax_sign.second);
+
+        // Explicitly construct row in error checking matrix
+        for (auto oax : {Axis::x, Axis::y, Axis::z})
+        {
+            explicit_mat[to_int(ax)][to_int(oax)] = [oax, new_ax_sign] {
+                if (oax != new_ax_sign.second)
+                    return 0;
+                if (new_ax_sign.first == '-')
+                    return -1;
+                return 1;
+            }();
+        }
     }
+    int det = determinant(explicit_mat);
+    CELER_VALIDATE(
+        det == +1,
+        << "invalid rotation matrix: determinant should be +1 but is " << det);
 }
 
 //---------------------------------------------------------------------------//
@@ -64,7 +84,7 @@ auto SignedPermutation::permutation() const -> SignedAxes
         result[ax].second = to_axis(temp & 0b11);
         temp >>= 2;
         // Push back "flip bit"
-        result[ax].first = temp & 0b1 ? msign : psign;
+        result[ax].first = temp & 0b1 ? '-' : '+';
         temp >>= 1;
     }
     return result;
@@ -91,11 +111,7 @@ SignedPermutation make_permutation(Axis ax, QuarterTurn theta)
 {
     CELER_EXPECT(ax < Axis::size_);
 
-    auto to_sign = [](int v) {
-        if (v < 0)
-            return SignedPermutation::msign;
-        return SignedPermutation::psign;
-    };
+    auto to_sign = [](int v) { return v < 0 ? '-' : '+'; };
 
     int const cost = cos(theta);
     int const sint = sin(theta);

diff --git a/src/orange/transform/SignedPermutation.hh b/src/orange/transform/SignedPermutation.hh
@@ -41,18 +41,13 @@ namespace celeritas
  * This allows the "rotate up" to simply copy one value at a time into a new
  * position, and optionally flip the sign of the result.
  *
- * TODO: implement error checking to prevent reflection.
+ * Construction of this class takes a length 3 array of \c SignedAxis values.
+ * The sign is a '+' or '-' character and the axis is the position of the
+ * nonzero component in that row.
  */
 class SignedPermutation
 {
   public:
-    //! Characters used for explicit construction
-    enum Sign : char
-    {
-        psign = '+',
-        msign = '-'
-    };
-
     //!@{
     //! \name Type aliases
     using SignedAxis = std::pair<char, Axis>;
@@ -99,9 +94,15 @@ class SignedPermutation
   private:
     // At least 16 bits: more than enough to round trip through a float
     using UIntT = short unsigned int;
-    static CELER_CONSTEXPR_FUNCTION UIntT max_value() { return (1 << 9) - 1; }
+
+    //// DATA ////
 
     UIntT compressed_;
+
+    //// FUNCTIONS ////
+
+    // Maximum compressed integer value used for bounds checking
+    static CELER_CONSTEXPR_FUNCTION UIntT max_value() { return (1 << 9) - 1; }
 };
 
 //---------------------------------------------------------------------------//

diff --git a/test/orange/transform/SignedPermutation.test.cc b/test/orange/transform/SignedPermutation.test.cc
@@ -31,13 +31,12 @@ std::string to_string(SignedPermutation::SignedAxes const& p)
 //---------------------------------------------------------------------------//
 }  // namespace
 
-//---------------------------------------------------------------------------//
-
 //---------------------------------------------------------------------------//
 /*!
  * Test harness for signed permutation.
  *
- * Note that the rotation matrix is a quarter turn around Z, then a hal
+ * Note that the rotation matrix is a quarter turn around Z, then a quarter
+ * turn around X.
  \code
     auto r = make_rotation(Axis::x, Turn{0.25},
                            make_rotation(Axis::z, Turn{0.25}));
@@ -74,6 +73,28 @@ TEST_F(SignedPermutationTest, construction)
     }
 }
 
+TEST_F(SignedPermutationTest, invalid_construction)
+{
+    {
+        SCOPED_TRACE("invalid sign");
+        EXPECT_THROW(SignedPermutation(SignedAxes{
+                         {{'?', Axis::y}, {'-', Axis::z}, {'+', Axis::x}}}),
+                     RuntimeError);
+    }
+    {
+        SCOPED_TRACE("singular matrix");
+        EXPECT_THROW(SignedPermutation(SignedAxes{
+                         {{'?', Axis::y}, {'-', Axis::z}, {'+', Axis::y}}}),
+                     RuntimeError);
+    }
+    {
+        SCOPED_TRACE("reflecting is prohibited");
+        EXPECT_THROW(SignedPermutation(SignedAxes{
+                         {{'+', Axis::y}, {'-', Axis::z}, {'+', Axis::x}}}),
+                     RuntimeError);
+    }
+}
+
 TEST_F(SignedPermutationTest, transform)
 {
     {