Repository: DCC-Lab/RayTracing

Issue:
Matrix multiplication returns a matrix with n1 = n2 = 1
I don't really know what we should do: in some cases, both matrices have the same n1 and n2, then the result should have n1 and n2. But sometimes they differ. I saw it happen in `MatrixGroup` when we append and build a transfer matrix: we create an identity matrix, but it has n1 = n2 = 1, and when we multiply with an element of the group (which can have n1 != n2 and both != 1), the product returns a matrix with the right ABCD components, back and front vertex, but don't have the right indices of refraction.

Since `MatrixGroup` already checks if there is a mismatch between indices, we could apply the indices of the right-hand side (or left-hand side, or the one that is not 1, I don't know) of the product. But at the same time we assume something, hence `__mul__` becomes less general. @dccote what do you think we should do?

Here is an example:
````python
elements = []
elements.append(DielectricInterface(n1=1, n2=n1, R=R1, diameter=diameter))
elements.append(Space(d=tc1, n=n1))
elements.append(DielectricInterface(n1=n1, n2=n2, R=R2, diameter=diameter))
elements.append(Space(d=tc2, n=n2))
elements.append(DielectricInterface(n1=n2, n2=1, R=R3, diameter=diameter))
group = MatrixGroup(elements) # This fails because of the transfer matrix in append
````
`append` code:
````python
def append(self, matrix):
    transferMatrix = Matrix(A=1, B=0, C=0, D=1)
        distance = upTo
        for element in self.elements:
            if element.L <= distance:
                transferMatrix = element * transferMatrix # This is the problem, * keeps default indices value
                distance -= element.L
            else:
                transferMatrix = element.transferMatrix(upTo=distance) * transferMatrix
                break

        return transferMatrix
````
`mul_matrix` code
````python
def mul_matrix(self, rightSideMatrix):
    a = self.A * rightSideMatrix.A + self.B * rightSideMatrix.C
    b = self.A * rightSideMatrix.B + self.B * rightSideMatrix.D
    c = self.C * rightSideMatrix.A + self.D * rightSideMatrix.C
    d = self.C * rightSideMatrix.B + self.D * rightSideMatrix.D
    L = self.L + rightSideMatrix.L
    fv = rightSideMatrix.frontVertex
    if fv is None and self.frontVertex is not None:
        fv = rightSideMatrix.L + self.frontVertex

    if self.backVertex is not None:
        bv = rightSideMatrix.L + self.backVertex
    else:
        bv = rightSideMatrix.backVertex

    return Matrix(a, b, c, d, frontVertex=fv, backVertex=bv, physicalLength=L) # We should not keep n1 = n2 = 1

````

Discussion:
GabGabG: See updated first comment / description

Patch:
diff --git a/raytracing/matrix.py b/raytracing/matrix.py
index ff6293f..5878f23 100644
--- a/raytracing/matrix.py
+++ b/raytracing/matrix.py
@@ -136,6 +136,10 @@ class Matrix(object):
         self.isFlipped = False
         super(Matrix, self).__init__()
 
+    @property
+    def isIdentity(self):
+        return self.A == 1 and self.D == 1 and self.B == 0 and self.C == 0
+
     @property
     def determinant(self):
         """The determinant of the ABCD matrix is always frontIndex/backIndex,
@@ -181,7 +185,7 @@ class Matrix(object):
                 "Unrecognized right side element in multiply: '{0}'\
                  cannot be multiplied by a Matrix".format(rightSide))
 
-    def mul_matrix(self, rightSideMatrix):
+    def mul_matrix(self, rightSideMatrix: 'Matrix'):
         r""" This function is used to combine two elements into a single matrix.
         The multiplication of two ABCD matrices calculates the total ABCD matrix of the system.
         Total length of the elements is calculated (z) but apertures are lost. We compute
@@ -269,7 +273,17 @@ class Matrix(object):
         else:
             bv = rightSideMatrix.backVertex
 
-        return Matrix(a, b, c, d, frontVertex=fv, backVertex=bv, physicalLength=L)
+        if self.isIdentity:  # If LHS is identity, take the other's indices
+            fIndex = rightSideMatrix.frontIndex
+            bIndex = rightSideMatrix.backIndex
+        elif rightSideMatrix.isIdentity:  # If RHS is identity, take other's indices
+            fIndex = self.frontIndex
+            bIndex = self.backIndex
+        else:  # Else, take the "first one" front index and the "last one" back index (physical first and last)
+            fIndex = rightSideMatrix.frontIndex
+            bIndex = self.backIndex
+
+        return Matrix(a, b, c, d, frontVertex=fv, backVertex=bv, physicalLength=L, frontIndex=fIndex, backIndex=bIndex)
 
     def mul_ray(self, rightSideRay):
         r"""This function does the multiplication of a ray by a matrix.
@@ -332,16 +346,19 @@ class Matrix(object):
         """
 
         outputRay = Ray()
-        outputRay.y = self.A * rightSideRay.y + self.B * rightSideRay.theta
-        outputRay.theta = self.C * rightSideRay.y + self.D * rightSideRay.theta
 
-        outputRay.z = self.L + rightSideRay.z
-        outputRay.apertureDiameter = self.apertureDiameter
+        if rightSideRay.isNotBlocked:
+            outputRay.y = self.A * rightSideRay.y + self.B * rightSideRay.theta
+            outputRay.theta = self.C * rightSideRay.y + self.D * rightSideRay.theta
+            outputRay.z = self.L + rightSideRay.z
+            outputRay.apertureDiameter = self.apertureDiameter
 
-        if abs(rightSideRay.y) > abs(self.apertureDiameter / 2.0):
-            outputRay.isBlocked = True
+            if abs(rightSideRay.y) > abs(self.apertureDiameter / 2.0):
+                outputRay.isBlocked = True
+            else:
+                outputRay.isBlocked = rightSideRay.isBlocked
         else:
-            outputRay.isBlocked = rightSideRay.isBlocked
+            outputRay = rightSideRay
 
         return outputRay
 
@@ -576,8 +593,8 @@ class Matrix(object):
         other elements there may be more.  For groups of elements, there can be any 
         number of rays in the list.
 
-		If you only care about the final ray that has propagated through, use 
-		`traceThrough()`
+        If you only care about the final ray that has propagated through, use 
+        `traceThrough()`
         """
 
         rayTrace = []
@@ -1547,7 +1564,7 @@ class Space(Matrix):
         """
         distance = upTo
         if distance < self.L:
-            return Space(distance)
+            return Space(distance, self.frontIndex)
         else:
             return self
 


Test patch:
diff --git a/raytracing/tests/testsMatrix.py b/raytracing/tests/testsMatrix.py
index 58e8738..267ae0a 100644
--- a/raytracing/tests/testsMatrix.py
+++ b/raytracing/tests/testsMatrix.py
@@ -36,6 +36,42 @@ class TestMatrix(envtest.RaytracingTestCase):
         self.assertEqual(m3.C, 1 * 7 + 3 * 8)
         self.assertEqual(m3.D, 2 * 7 + 4 * 8)
 
+    def testIsIdentity(self):
+        m = Matrix()
+        self.assertTrue(m.isIdentity)
+
+    def testIsNotIdentity(self):
+        m = Matrix(1, 2, 0, 1)
+        self.assertFalse(m.isIdentity)
+
+    def testMatrixProductIndicesBoth1(self):
+        m1 = Matrix()
+        m2 = Matrix()
+        m3 = m1 * m2
+        self.assertEqual(m3.frontIndex, 1)
+        self.assertEqual(m3.backIndex, 1)
+
+    def testMatrixProductIndicesLHSIsIdentity(self):
+        m1 = Matrix(backIndex=1.33)
+        m2 = Matrix(1, 10, 0, 1, frontIndex=1.5, backIndex=1.5)
+        m3 = m1 * m2
+        self.assertEqual(m3.frontIndex, 1.5)
+        self.assertEqual(m3.backIndex, 1.5)
+
+    def testMatrixProductIndicesRHSIsIdentity(self):
+        m1 = Matrix(backIndex=1.33)
+        m2 = Matrix(1, 10, 0, 1, frontIndex=1.5, backIndex=1.5)
+        m3 = m2 * m1
+        self.assertEqual(m3.frontIndex, 1.5)
+        self.assertEqual(m3.backIndex, 1.5)
+
+    def testMatrixProductIndicesNoIdentity(self):
+        m1 = Matrix(1, 10, 0, 1, backIndex=1.33, frontIndex=1)
+        m2 = Matrix(1, 10, 0, 1, backIndex=1, frontIndex=1.33)
+        m3 = m2 * m1
+        self.assertEqual(m3.frontIndex, 1)
+        self.assertEqual(m3.backIndex, 1)
+
     def testMatrixProductWithRayMath(self):
         m1 = Matrix(A=1, B=2, C=3, D=4)
         rayIn = Ray(y=1, theta=0.1)
@@ -323,7 +359,8 @@ class TestMatrix(envtest.RaytracingTestCase):
         # One less ray, because last is blocked
         self.assertEqual(len(traceManyThrough), len(rays) - 1)
 
-    @envtest.skipIf(sys.platform == 'darwin' and sys.version_info.major == 3 and sys.version_info.minor <= 7,"Endless loop on macOS")
+    @envtest.skipIf(sys.platform == 'darwin' and sys.version_info.major == 3 and sys.version_info.minor <= 7,
+                    "Endless loop on macOS")
     # Some information here: https://github.com/gammapy/gammapy/issues/2453
     def testTraceManyThroughInParallel(self):
         rays = [Ray(y, y) for y in range(5)]
@@ -336,7 +373,8 @@ class TestMatrix(envtest.RaytracingTestCase):
         except:
             pass
 
-    @envtest.skipIf(sys.platform == 'darwin' and sys.version_info.major == 3 and sys.version_info.minor <= 7,"Endless loop on macOS")
+    @envtest.skipIf(sys.platform == 'darwin' and sys.version_info.major == 3 and sys.version_info.minor <= 7,
+                    "Endless loop on macOS")
     # Some information here: https://github.com/gammapy/gammapy/issues/2453
     def testTraceManyThroughInParallel(self):
         rays = [Ray(y, y) for y in range(5)]