Introduce the manifold of symmetric positive definite matrices with two metrics #27
Conversation
|
An and I already see, I'd also have to merge from #22 to get the CI running. |
|
And fix the file name ... 😄 |
|
Yes, I was again too fast and fetched a cold two days ago -.- |
I think we can do something similar to how different retraction methods are implemented:
#22 changes a lot and I think it would be OK to just merge it soon. I would like to have this resolved though: #22 (comment) . |
|
That sounds good, to specify the type (and maybe default that to parallel transport). The CI idea was mainly for me to keep track of :) |
…transfers parallel transport.
There was a problem hiding this comment.
Just a few comments. It seems generally favorable to use eigen instead of svd, since the Vt factor is never used anyway and is equal to transpose(U) by symmetry. Moreover, I'd suggest to use Symmetric whenever you know something is symmetric. That is a simple wrapper, which is fast to construct, in contrast to numerical symmetrization by n^2 additions à la T + transpose(T), whose symmetry would have to be obtained from yet another n*(n+1)/2 checks.
Codecov Report
@@ Coverage Diff @@
## master #27 +/- ##
===========================================
- Coverage 72.95% 59.68% -13.28%
===========================================
Files 10 9 -1
Lines 599 439 -160
===========================================
- Hits 437 262 -175
- Misses 162 177 +15
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #27 +/- ##
==========================================
+ Coverage 82.45% 83.62% +1.17%
==========================================
Files 16 18 +2
Lines 986 1136 +150
==========================================
+ Hits 813 950 +137
- Misses 173 186 +13
Continue to review full report at Codecov.
|
|
I just simplified most numerical |
|
And I noticed that |
|
As far as I can see |
… are covered by ManifoldsBase in the future.
…he strict versions.
|
OK, I'll look at it later and add these missing methods. |
… work with the `hasMetric` trait.
…arting with log since thats the first error).
|
This branch now works with the current version of |
kellertuer
changed the title
Concerning the comparison to Matlab – without a thorough test, the |
|
Sorry for my incomplete comments, and the additional work it triggers. I think it may be actually advantageous to have a test-passing version merged, and then come back and add some optimizations via PRs, with CI running and testing the proposed changes. I don't mean to hold this up by any means. |
|
I am very thankful for all your comments and discussions. I think up to the Julia version check thing, I got most of it optimised (even the |
This is the manifold of SPD matrices with
and I already have a question before I start documentation and testing, namely
vector_transport: Is that meant to beparallel_transportor more general than that? If the second one is the case, then how do I specify a parallel transport?Feel free to already revise existing code :)