JuliaManifolds / Manifolds.jl

Manifolds.jl provides a library of manifolds aiming for an easy-to-use and fast implementation.
https://juliamanifolds.github.io/Manifolds.jl
MIT License
373 stars 55 forks source link

Move ManifoldDiff extension here #623

Closed mateuszbaran closed 1 year ago

mateuszbaran commented 1 year ago

See here: https://github.com/JuliaManifolds/ManifoldDiff.jl/issues/20

codecov[bot] commented 1 year ago

Codecov Report

Merging #623 (b70bf1b) into master (c6ec0a5) will increase coverage by 0.04%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #623      +/-   ##
==========================================
+ Coverage   98.96%   99.01%   +0.04%     
==========================================
  Files         104      104              
  Lines       10046    10102      +56     
==========================================
+ Hits         9942    10002      +60     
+ Misses        104      100       -4     
Impacted Files Coverage Δ
src/Manifolds.jl 86.66% <ø> (ø)
src/groups/group.jl 100.00% <100.00%> (ø)
src/manifolds/Circle.jl 100.00% <100.00%> (ø)
src/manifolds/Euclidean.jl 99.54% <100.00%> (+0.01%) :arrow_up:
src/manifolds/Hyperbolic.jl 100.00% <100.00%> (ø)
src/manifolds/ProductManifold.jl 96.59% <100.00%> (+0.13%) :arrow_up:
src/manifolds/Sphere.jl 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

mateuszbaran commented 1 year ago

These new lines most likely won't be cover until ManifoldDiff is merged, and tests in that PR likely won't pass until this is merged. Which one do you prefer to merge and tag first?

kellertuer commented 1 year ago

For me its ore reasonable to remove it from ManifoldDiff first and then add it here, since otherwise there is a version where the extension exists twice.

mateuszbaran commented 1 year ago

I have no idea why Manopt gradient tests fail here. Locally they pass fine.

kellertuer commented 1 year ago

The error message seems to be quite Diff-related though

mateuszbaran commented 1 year ago

Yes, it doesn't see the adjoint_Jacobi_field method for Circle. That method was in the extension of ManifoldDiff. We have it in this PR now and it's covered by a test so this makes no sense to me.

kellertuer commented 1 year ago

Ah, maybe Manopt still checks our some old version? Because currently Manopt/ManifoldDiff/Manifolds in their most recent version do not play well together, since the extension is nowhere (reverted to 0.3.2 to get it working again). So I am not sure how to resolve this. Register this version and hope for the best?

kellertuer commented 1 year ago

The same happens for now on Manopt CI, I think the choice of having a version combination without this extension was not that good and a next release here should fix that.

On the other hand it means that there is now combinations of Manifolds and ManifoldDiff that do not work well together (one or two combinations where the extension is nowhere to be found), but I think this can not be avoided.

kellertuer commented 1 year ago

I think I found a solution. I will restrict ManifoldDiff in Manopt to 0.3.2, then it should work fine; we register the stochastic PR (maybe also together with the new tutorial update?) as a new version (with the 0.3.2 bound). Then this PR defines a few functions double for Manopt, but that should be fine; afterwards bumping version to 0.3.3 and Manifolds <= this PR should also make Manopt work again.

mateuszbaran commented 1 year ago

OK, that sounds like a good plan.

kellertuer commented 1 year ago

It worked :) After this is merged and registered I can use the version number from here and Diff 0.3.3 as lower bounds in Manopt, for the next version.

mateuszbaran commented 1 year ago

Great! So I think this PR can be merged now?

kellertuer commented 1 year ago

Yes :)