MachineLearningLifeScience / stochman

Algorithms for computations on random manifolds made easier
Apache License 2.0
86 stars 11 forks source link

Possible bug in stochman.curves.CubicSpline #29

Open fadel opened 1 year ago

fadel commented 1 year ago

Hi,

I was going through the source code of stochman with a colleague and we found what could likely be a bug in stochman.curves.CubicSpline. In essence, the code for computing the coefficients used in the second-order derivative seems to have the wrong position for the coefficients.

I would suggest the following patch:

diff --git a/stochman/curves.py b/stochman/curves.py
index 97c22bd..a40f7ca 100644
--- a/stochman/curves.py
+++ b/stochman/curves.py
@@ -356,7 +356,7 @@ class CubicSpline(BasicCurve):
             second = torch.zeros(num_edges - 1, 4 * num_edges, dtype=self.begin.dtype)
             for i in range(num_edges - 1):
                 si = 4 * i  # start index
-                fill = torch.tensor([0.0, 0.0, 6.0 * t[i], 2.0], dtype=self.begin.dtype)
+                fill = torch.tensor([0.0, 0.0, 2.0, 6.0 * t[i]], dtype=self.begin.dtype)
                 second[i, si : (si + 4)] = fill
                 second[i, (si + 4) : (si + 8)] = -fill

In our (limited) tests, using this version leads to more accurate logmaps.

What do you think? Was that intentional?