Define Meta Manifolds already in ManifoldsBase #169
Conversation
Codecov Report
@@ Coverage Diff @@
## master #169 +/- ##
========================================
Coverage 99.96% 99.96%
========================================
Files 20 26 +6
Lines 2562 3160 +598
========================================
+ Hits 2561 3159 +598
Misses 1 1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Hmm... since it's breaking, maybe let's move the implementation of fiber bundles from JuliaManifolds/Manifolds.jl#642 here? And Product manifolds as well. |
|
Sure, why not. I was not aware you worked on the Fibre Bundle in that PR. |
|
It's in the PR's title 😄 |
|
Will you make the update or prefer me to do that? |
|
Since those are your changes (and it is harder for me to track them down in your PR), maybe you could add them here? Feel free to add product and power as much as possible here, too, I'd love to have them here as well. |
|
OK, sure, I will do it then. |
|
What do you think about the extension here? Or should the few functions using |
|
I prefer to have that extension here. |
|
Nice, I do not have a strong preference, but I agree, that this puts all VectorBundle-Fun in one place. Then Iwe should add the Revise dependency here as well (that's what's missing still). |
|
I just spend quite a bit of time with the docs. Citations should be fine already – but the docs still have 8 doctorings that we never included somewhere. I only fixed about a few and these are still missing. Test coverage – I started with that but I am not yet sure how far I got. |
|
Maybe we should also rethink the docs a bit when fixing the remaining ones – the manifolds section is by now very long – and might be better to be restructured. |
|
Maybe let's update docs when new things are ready? I'm in the middle of moving metamanifolds here. |
|
Sure, that's one of the reasons I waited with fixing :) |
kellertuer
changed the title
|
Sounds good 👍 . I more thing I'd like to do before merging this is making all tests pass (locally) in the Manifolds.jl rework PR. |
|
Ah, that sounds like a very good idea 👍 In the mean time I can do the error-keyword-rework already for #135 – that also mainly seems to be a few lines of code and a bit of test adaptions. |
|
Sure, that should be fairly simple. |
Well, ± ambiguities and a few internals for embedded manifold or such that did change. but I think I am nearly there. |
|
I've updated |
…uctRetraction and ProductVectorTransport, move × for productManifold here.
Instead of a map one could also allocate n copies of the retraction, but maybe this is easiest.
|
I think I found a way to introduce retractions with non-product retractions as discussed in JuliaManifolds/Manifolds.jl#659, the only “disadvantage” might be, that when margin this into #167. we might have to repeat these for the This might be slightly bridging our layers, but layer one says “Note that all other parameters of a function should be as least typed as possible for all parameters besides the manifold.” so that is what I did here for the abstract retraction case and put it on layer 1; currently I also moved the this is still missing a bit of testing, but I will first check which lines are missing, since also the new |
OK, if this doesn't cause any ambiguities then I'm fine with it. |
That is exactly why I moved them from L2 to L1, on L2 they caused too many (non-ignorable) ambiguities. I will then write a few tests now :) |
|
Oh, two of the function definitions I do not yet hit but I thought I would have. I will only have time tomorrow to check for these as well. edit: I like what this turned out to be, also the ne |
|
Cool. With this latest fix, test are passing in Manifolds.jl PR. I will continue with docs and coverage there. |
…anifoldsBase.jl into kellertuer/VectorBundle
|
Thanks for fixing one of my a-bit-too-fast copied method. I just extended (hopefully finally) test coverage. I think now this should be good to go :) |
|
We have one false-positive uncovered line left, so code coverage is as best as possible. |
|
Yes, I think this is pretty much ready and we can work on releasing 0.15.0. I think first, we need to merge all three breaking PRs into one branch and check if the Manifolds.jl PR needs some additional work. |
|
Cool. Then let's start with this one, then retractions (since we might want to check that this still works with the new dispatch) – and then the error PR. |
This allows the user to even use a TangentSpaceAtPoint when only depending on ManifoldsBase.
I would like to have this for defining proper/nice/generic subproblems in tangent spaces in Manopt.jl (e.g. for the current trust region rework).
This is breaking (hence also for 0.15) in the sense that with this version otherwise the VectorBundle is defined twice (which kills precompiation).
To do
ArrayPartitionsections to a weak dependency withRecursiveArrayTools.