Fixes a bug, that occured when calling exp!(Rotations(2), p, p, X) since q=p was modified.
#392
Conversation
…since `q=p` was modified.
Co-authored-by: Mateusz Baran <[email protected]>
Co-authored-by: Mateusz Baran <[email protected]>
test/rotations.jl
Outdated
| @@ -62,6 +62,12 @@ include("utils.jl") | |||
| v = log(M, pts[1], pts[2]) | |||
| @test norm(M, pts[1], v) ≈ (angles[2] - angles[1]) * sqrt(2) | |||
|
|
|||
| # check that exp! does not have a side effect | |||
| q = copy(M, pts[1]) | |||
There was a problem hiding this comment.
We could actually move this test to general manifold tests. It is something Manopt.jl assumes so checking it for all manifolds makes sense IMO.
There was a problem hiding this comment.
Manopt.jl would be maybe a little generic, actually I always assumed that that is the case ;)
There was a problem hiding this comment.
Since you also opened an Issue for the generic case, maybe let‘s solve it in a generic separate PR?
There was a problem hiding this comment.
OK, sure, I'll try that in a separate PR in the evening today or tomorrow.
There was a problem hiding this comment.
Hm, we do not have the copy you proposed to use.
There was a problem hiding this comment.
Weird, I think we should have that copy.
There was a problem hiding this comment.
I agree, we should, since we have copyto!, but maybe lets do a new PR (even in ManifoldsBase?) it would just combining existing methods.
There was a problem hiding this comment.
Sure, it's an issue for ManifoldsBase.jl.
I might have mixed up row- and column major indexing and hence have introduced the rotation the wrong direction around.
fix allocation of `q`.
Codecov Report
@@ Coverage Diff @@
## master #392 +/- ##
==========================================
- Coverage 97.92% 97.92% -0.01%
==========================================
Files 76 76
Lines 5919 5914 -5
==========================================
- Hits 5796 5791 -5
Misses 123 123
Continue to review full report at Codecov.
|
So the
exp!returned a wrong result when called asexp!(M, p, p X)due to this side effect. Also adds a test for this.