-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Polish the Differential Interfaces #429
Conversation
edit: To avoid confusion with git commit comments from other branches I cleared this one by squashing the work until now into one commit. edit2: I then had to do the same strange merge conflict from yesterday again. Squashing and merging is really strange sometimes. I will not merge another dev branch ever again ;) |
commit 26ad8a5 Author: Ronny Bergmann <[email protected]> Date: Thu Sep 23 20:17:01 2021 +0200 runs formatter. commit 99bcb23 Merge: b027f4c 2a03fb1 Author: Ronny Bergmann <[email protected]> Date: Thu Sep 23 18:33:25 2021 +0200 Merge branch 'master' into kellertuer/polish-diff-interfaces commit 2a03fb1 Author: Mateusz Baran <[email protected]> Date: Thu Sep 23 18:08:23 2021 +0200 Support for Zygote and ReverseDiff gradients (#427) * Support for Zygote and ReverseDiff gradients * Add Zygote test dependency * bump ambiguity limit because of Zygote * fix tests and Zygote backend * bump Julia to 1.5 * fixing some issues on Julia 1.7-rc1 * more fixing for Julia 1.7 * formatting * reduce tangent vector length in a test since the approximation only works very locally (and they changed the default random number generator) * Update Project.toml Co-authored-by: Ronny Bergmann <[email protected]> * bump atol on Rotations * reduce tangent vector size even further. * adapt one more tolerance Co-authored-by: Ronny Bergmann <[email protected]> commit b027f4c Author: Ronny Bergmann <[email protected]> Date: Thu Sep 23 17:11:19 2021 +0200 fix a few typos. commit dcc471e Author: Ronny Bergmann <[email protected]> Date: Thu Sep 23 13:53:07 2021 +0200 unify naming. commit 3de9a30 Author: Ronny Bergmann <[email protected]> Date: Thu Sep 23 13:33:59 2021 +0200 Reduce dependencies, wort a little bit further on polishing. commit 8702902 Author: Ronny Bergmann <[email protected]> Date: Thu Sep 23 10:45:36 2021 +0200 runs formatter. commit 378bbba Author: Ronny Bergmann <[email protected]> Date: Thu Sep 23 10:45:15 2021 +0200 fix the other jacobian. commit a7d1edb Author: Ronny Bergmann <[email protected]> Date: Thu Sep 23 10:29:37 2021 +0200 renaming and dependencies. commit 02f97d6 Merge: aca6d5c 9fc772a Author: Ronny Bergmann <[email protected]> Date: Wed Sep 22 19:48:36 2021 +0200 Merge branch 'master' into kellertuer/polish-diff-interfaces # Conflicts: # Project.toml # src/Manifolds.jl commit aca6d5c Author: Ronny Bergmann <[email protected]> Date: Wed Sep 22 19:46:59 2021 +0200 Add a proper working default. commit 9fc772a Author: Ronny Bergmann <[email protected]> Date: Wed Sep 22 19:46:01 2021 +0200 Sketch change metric. (#423) * Adds a change_metric and a change_representer * Apply suggestions from code review * bump version. Co-authored-by: Mateusz Baran <[email protected]> commit 1462018 Merge: 923a718 4501a27 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 22 19:20:52 2021 +0200 Merge branch 'kellertuer/gradient-conversion' into kellertuer/polish-diff-interfaces # Conflicts: # Project.toml # src/differentiation/riemannian_diff.jl commit 923a718 Merge: d391483 07c905e Author: Ronny Bergmann <[email protected]> Date: Wed Sep 22 18:20:39 2021 +0200 Merge branch 'mbaran/reverse-diff-zygote' into kellertuer/polish-diff-interfaces commit 07c905e Author: Mateusz Baran <[email protected]> Date: Wed Sep 22 17:27:10 2021 +0200 Update Project.toml Co-authored-by: Ronny Bergmann <[email protected]> commit 4501a27 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 22 17:19:42 2021 +0200 Apply suggestions from code review Co-authored-by: Mateusz Baran <[email protected]> commit 3c47350 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 22 08:59:28 2021 +0200 reduce tangent vector length in a test since the approximation only works very locally (and they changed the default random number generator) commit 3b7396d Author: Mateusz Baran <[email protected]> Date: Tue Sep 21 19:19:09 2021 +0200 formatting commit 5d75e6e Author: Mateusz Baran <[email protected]> Date: Tue Sep 21 19:17:28 2021 +0200 more fixing for Julia 1.7 commit c723345 Author: Mateusz Baran <[email protected]> Date: Tue Sep 21 12:31:30 2021 +0200 fixing some issues on Julia 1.7-rc1 commit cf00e22 Author: Mateusz Baran <[email protected]> Date: Tue Sep 21 08:49:40 2021 +0200 bump Julia to 1.5 commit 1ed46db Author: Mateusz Baran <[email protected]> Date: Mon Sep 20 23:02:03 2021 +0200 fix tests and Zygote backend commit 7849fc0 Author: Mateusz Baran <[email protected]> Date: Mon Sep 20 19:45:41 2021 +0200 bump ambiguity limit because of Zygote commit 9163480 Author: Mateusz Baran <[email protected]> Date: Mon Sep 20 14:40:50 2021 +0200 Add Zygote test dependency commit 7ed57da Author: Mateusz Baran <[email protected]> Date: Mon Sep 20 12:48:09 2021 +0200 Support for Zygote and ReverseDiff gradients commit 2f498da Author: Ronny Bergmann <[email protected]> Date: Sat Sep 18 23:17:32 2021 +0200 add a test that double metric wrapping is avoided; remove unnecessary dispatch. commit e180bc4 Author: Ronny Bergmann <[email protected]> Date: Sat Sep 18 21:58:49 2021 +0200 Simplify default implementation. Extend coverage. Order functions alphabetically. commit 420047d Author: Ronny Bergmann <[email protected]> Date: Sat Sep 18 20:07:21 2021 +0200 Fix the error message for hyperbolic. commit ed2871c Author: Ronny Bergmann <[email protected]> Date: Sat Sep 18 19:54:19 2021 +0200 maybe fix the new test by slightly overtyping the basis. commit 49dc500 Author: Ronny Bergmann <[email protected]> Date: Fri Sep 17 18:44:59 2021 +0200 format this interims non-working test. commit a78ace4 Author: Ronny Bergmann <[email protected]> Date: Fri Sep 17 18:40:57 2021 +0200 Maybe. just maybe fixed local metric, but now get vector and get coordinates do not work for the test. commit 97417ae Author: Ronny Bergmann <[email protected]> Date: Thu Sep 16 14:31:49 2021 +0200 Improve docs. commit d34ef6c Merge: 95a3c8b d1dbb7c Author: Ronny Bergmann <[email protected]> Date: Thu Sep 16 11:09:24 2021 +0200 Merge branch 'kellertuer/gradient-conversion' of github.com:JuliaManifolds/Manifolds.jl into kellertuer/gradient-conversion commit 95a3c8b Author: Ronny Bergmann <[email protected]> Date: Thu Sep 16 11:09:16 2021 +0200 adds a test for positive vectors and starts testing the default Metric fallbacks using the local metric. commit d1dbb7c Author: Ronny Bergmann <[email protected]> Date: Thu Sep 16 10:47:57 2021 +0200 Apply suggestions from code review Co-authored-by: Mateusz Baran <[email protected]> commit 14fdb06 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 22:39:38 2021 +0200 bump version. commit cbcef94 Merge: da98b5f bd2b425 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 22:38:45 2021 +0200 Merge branch 'master' into kellertuer/gradient-conversion commit da98b5f Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 22:37:18 2021 +0200 append this step to riemannian differentiation. commit c154516 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 22:37:01 2021 +0200 finish tests for product and power – extend features to also work on the sphere. commit 844f8c4 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 21:42:59 2021 +0200 Fixes a few typos in the docs. commit 5005965 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 20:40:11 2021 +0200 Document and test SPDs commit d2cec4d Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 20:30:00 2021 +0200 Finish positive Numbers. commit 7737c92 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 20:03:49 2021 +0200 Extend ProbabilitySimples to also cover change_metric. commit 6300f80 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 20:03:33 2021 +0200 delete change_representer since it should be inherited from ProbabilitySimplex combined with AbstractPowerManifold and EmbeddedManifold. commit 95b8bc6 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 19:48:20 2021 +0200 finishes testing for hyperbolic. commit 6f1fb64 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 19:47:37 2021 +0200 fix a small bug in Hyperbolic copyto! which returned the wrong value. commit a62e134 Author: Ronny Bergmann <[email protected]> Date: Wed Sep 15 10:31:24 2021 +0200 Documentation and Test for Poincaré Ball conversion. commit d84a3cc Author: Ronny Bergmann <[email protected]> Date: Tue Sep 14 19:52:52 2021 +0200 adds two further small tests. commit ea76128 Author: Ronny Bergmann <[email protected]> Date: Tue Sep 14 19:21:08 2021 +0200 Document and test hyperbolic. commit 257b223 Author: Ronny Bergmann <[email protected]> Date: Tue Sep 14 18:20:55 2021 +0200 Finish documentation and test for Generalised Grassmann. commit 152e9a1 Author: Ronny Bergmann <[email protected]> Date: Tue Sep 14 18:20:34 2021 +0200 fix transparency of nonmutating to parent on a Manifold level. commit 1b79065 Author: Ronny Bergmann <[email protected]> Date: Mon Sep 13 17:49:20 2021 +0200 Fix two typos. commit 2eac3ee Author: Ronny Bergmann <[email protected]> Date: Mon Sep 13 16:30:35 2021 +0200 Change `change_gradient` to `change_represender` and introduce mutating variant commit 5539f4b Author: Ronny Bergmann <[email protected]> Date: Mon Sep 13 12:11:52 2021 +0200 introduce mutating variants and decorator transparency. commit 714862e Merge: 4912e4f 05968dc Author: Ronny Bergmann <[email protected]> Date: Sun Sep 12 20:09:17 2021 +0200 Merge branch 'kellertuer/gradient-conversion' of github.com:JuliaManifolds/Manifolds.jl into kellertuer/gradient-conversion commit 4912e4f Author: Ronny Bergmann <[email protected]> Date: Sun Sep 12 20:09:11 2021 +0200 Adress points from code review. commit 05968dc Author: Ronny Bergmann <[email protected]> Date: Sun Sep 12 20:03:18 2021 +0200 Apply suggestions from code review Co-authored-by: Mateusz Baran <[email protected]> commit 9769a1a Author: Ronny Bergmann <[email protected]> Date: Sun Sep 12 18:17:45 2021 +0200 add the generic case for doubly stochastic (includes symmetric, too). commit 42875a6 Author: Ronny Bergmann <[email protected]> Date: Sun Sep 12 18:13:42 2021 +0200 implement the remaining conversions. commit fb304c2 Merge: cb2f932 2ee2072 Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 16:51:29 2021 +0200 Merge branch 'kellertuer/gradient-conversion' of github.com:JuliaManifolds/Manifolds.jl into kellertuer/gradient-conversion commit cb2f932 Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 16:51:17 2021 +0200 adds a few conversion, where the embedding is not isometric. commit 2ee2072 Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 13:04:01 2021 +0200 Update src/manifolds/MetricManifold.jl Co-authored-by: Mateusz Baran <[email protected]> commit c742ea5 Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 12:55:35 2021 +0200 Fix two typos. commit a17b4ca Merge: 540f374 056e24c Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 12:47:27 2021 +0200 Merge branch 'kellertuer/gradient-conversion' of github.com:JuliaManifolds/Manifolds.jl into kellertuer/gradient-conversion # Conflicts: # src/manifolds/MetricManifold.jl commit 540f374 Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 12:46:50 2021 +0200 Specify Reisz representer, change adjoint to ^H. commit 056e24c Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 12:38:40 2021 +0200 Update src/manifolds/MetricManifold.jl Co-authored-by: Mateusz Baran <[email protected]> commit 158bbf2 Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 12:33:10 2021 +0200 Apply suggestions from code review Co-authored-by: Mateusz Baran <[email protected]> commit 616db5d Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 10:39:47 2021 +0200 Fix a few typos in docs. commit c9737b3 Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 10:21:02 2021 +0200 adds a change_gradient. commit 0857b21 Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 09:25:54 2021 +0200 fix a few typos. commit a302733 Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 09:04:07 2021 +0200 rfF. commit 3347930 Author: Ronny Bergmann <[email protected]> Date: Sat Sep 11 09:01:30 2021 +0200 Document insights from yesterday evening and – getting stuck at the next point. commit ae0cdd1 Author: Ronny Bergmann <[email protected]> Date: Fri Sep 10 20:38:56 2021 +0200 Sketch change metric.
26ad8a5
to
4376b8b
Compare
# Conflicts: # Project.toml # src/Manifolds.jl # src/differentiation/differentiation.jl # src/differentiation/finite_diff.jl # src/differentiation/reverse_diff.jl # src/differentiation/riemannian_diff.jl # src/differentiation/zygote.jl # test/differentiation.jl
It looks like merging branches can be not only confusing for humans but for github as well 😉 . I think after this we can mark #42 as resolved; the intrinsic approach is a somewhat separate issue, tracked in JuliaManifolds/ManifoldDiff.jl#28 IIUC. |
Cool, I think so, too. Then this PR fixes even 3 issues. Nice! |
I noticed one challenge when I tried fixing the first errors. Currently we compute the new local metric jacobian here Manifolds.jl/src/manifolds/MetricManifold.jl Lines 471 to 480 in b2f2674
using a default retraction. This has a limit for the case where we solve the ODE for the geodesic/exp at Manifolds.jl/src/manifolds/ConnectionManifold.jl Lines 164 to 172 in b2f2674
which internally calls We have to avoid, that (for the case the local jacbian is used to compute the exp problem) that the |
Hm, that's an interesting problem. My guess would be that the only way to make this work is working in a chart, that is |
uff. I would prefer checking (before calling local_jacobian that the (explicit, non-default) retraction is not the exponential retraction in solve_ode_exp. That would be maybe be the easier fix? |
I don't see any retraction in |
That is, |
Of course not. Until now it used |
That is a very good idea since it only approximates exp, that solves the problem to that extend. |
OK, I thought that's not the issue we are solving here. It seems that |
Hm, then I though about a completely different way of solving this. Remove the but for your two approaches I have not yet understood how you mean to solve those, neither in theory for the charts with + not in practice for both. |
Could you explain how would you like to use a retraction in |
I would remove the default fallback of A retraction of type |
Yes, I agree that this is a good idea.
I don't quite see yet how you want to rewrite struct ChartRetraction{TA<:AbstractAtlas,TI} <: AbstractRetractionMethod
A::TA
i::TI
end
function retract(M, p, X, cr::ChartRetraction)
pc = get_parameters(M, cr.A, cr.i, p)
Xc = get_coordinates(M, p, X, InducedBasis(TangentSpaceType, A, i))
return get_point(M, cr.A, cr.i, pc + Xc)
end The problem is that it's currently only implemented for |
Ah that looks also nice and would be the inner retraction then, sure. |
Notice though that when you write a solver for geodesic using that retraction you can eliminate calling |
Yes, I see. One could implement a special solver for this case (dispatching on |
Yes, and I think that solver would be the easiest one to write (and the closest to the current implementation). |
Could you try to propose that? Because I think the generic one should be relatively close to what is done now actually (with the retraction inside instead of the +, sure your approach would really just use + in |
I think I could write that. Also, which + do you want to replace with retraction? Currently the addition (for performing a step) is done inside OrdinaryDiffEq.jl. |
Before we started this PR, there was basically a |
ON my way of fixing the ODE thingy, I noticed that both connection and metric decorator implement the jacobian-things (local metric, first, second), but while Metric just calls an ODE for the local metric jacobian and continues from there, the ConnectionManifold computes the second jacobians with an ODE (but neither first nor local metric jacobina9: Is that an inconsistency or intended? |
The test was expecting |
I ran into a next problem when starting the own ODE tests: In the version before this version, the ode-parameters were stored as points in the embedding, but now I thought it is necessary to reduce the size to the manifold_dimension? This is what the code currently does, but then there is a size mismatch when initialising the ODE Manifolds.jl/src/differentiation/ode.jl Lines 15 to 16 in a4a2716
so we still might habe to revise this to fit. Edit: Taking the representation size should work for now, but I again get stuck on local metrics. They are really something to hunt me with. Implementing it for the metric manifold did not work, implementing it for the manifold itself did not work, tried both with less or more parametrised bases – never tries to “find/catch” - I seem generally unable to “track” down the right signature for bases to be found in dispatch on them with all their parameters. |
I'm not sure. I only know how to do this ODE solving either in a chart or through ManifoldDiffEq.jl.
I'm fixing it, the main problem I think was that you forgot |
There are still errors but much further along ( |
Thanks for the help, I think in my many tries I had that variant, too, but I really seem to struggle a lot with things involving metric/basis combined here, especially which parameters to specify for the basis (I feel like bases really have many parameters, though it is also just 3 or something?). Sure there are more errors down the road, I will slowly work towards that. Is there a reason I can not ask the sphere for the metric? I had hoped it just has a metric tensor (namely the one from the embedding restricted)? |
Bases really have only two meaningful parameters: whether it's a real or complex basis and whether it's a basis of the tangent space or some other space. Probably the length of
The only reason is that no one has implemented that function yet 😄 . Metric tensor from the embedding can't work because:
|
My problem is I tried (a) TestSphere with the Basis (b) TestSphere with basis and one Parameter (c) two parameters (d) MetricManifold with basis (no parameters) e) one parameter, f) two parameters – ... got annoyed and left. That is my usual workflow. 6-8 tries, none works -> you fix it and then it looks like one that I already tried. I do not yet know what I am doing wrong in my near-infinite number of tries.
Ah these lazy developers again 😄
no in general that is too complicated but I might then add the local metric for the sphere just to reuse it here for the test sphere. |
I think the reasonable workflow would be:
That's a good idea 👍 |
Actually, we have the local metric already, since the Sphere has the |
Somewhere we still have some dimension mixup (i.e. whether we are in coordinates at a place or in the representation). Currently I somehow end up in a Edit computed the jacobian in a basis, so we have to convert dx as well, now all dimensions are correct, just the result is still wrong. Edit2: I know why: The extrinsic (embedded) Jacobian computation is now wrong in the generic setting. I think I have an Idea how to fix this, but I need some time to think about it. The idea is (a) in the embedded case: embed -> jacobian -> project/coordinates and (b) implement explicit formula in other cases. |
I am a little stuck (maybe I thought about it too much or something) – somehow I can not manage to write the solve_exp_ode correctly, currently I either end up with two things . Let a) write a variant that tries to work in a too large space (n) where the b) write a variant that works in the right space, then I am stuck in how to initialise the p (just zero?) and how to fill that part of the And in general I am not sure in which of the variants to work, since we should have a version (i.e. in some chart) where we can do a “Euclidean” ode, i.e. one where we have a plus right? I also just tried to write the fallback for |
…d (b) into its own test file (away from metric).
Yes, in this variant you need a different way of calculating the tangent vector for ODE and, unfortunately, I don't currently see a good way to do this.
The variant I've written in #431 does exactly that:
I think we may wait with that, currently the preferred way of working in Manifolds.jl is to implement explicit retractions, and the support for ODE-based retractions is very experimental. I'd wait with making it the default for non-metric manifolds until we have it at least somewhat tested and benchmarked on a simple problem (such as geodesics on an ellipsoid). |
Thanks for the help. I will take the approach (a) back to what we had, just generalise it to (isometrically) embedded sub manifolds where I think I have an idea (and then getting to finish this PR and just rewriting/reorganising the tests a little bit). For the generic fallback my idea stems more from the fact that for the default metrics we “strip away” the metric part anyways (in most cases), since we assume to have the implicit (first/default) metric to be implemented without the metric decorator. |
OK, I'll wait until you try your idea 🙂 .
I think that could work but you'd still need ManifoldDiffEq.jl instead of OrdinaryDiffEq.jl, right? |
Currently 26 tests fail in metric, and I have no clue why, because I do not understand what they test, since there is no explanations around and code without comments is a little hard to reverse-engineer. So I am not sure how to continue here – revert all the ODE stuff and just keep the renaming of the embedded AD stuff? Omit this PR totally? The last part would be a little sad since I spent really a lot of time on this. Even the first is a little sad, but I fear I do not understand enough of the ODE and how to do it in a basis or the embedding to fix it here. |
Looking at that code, I would be surprised if it made sense to you 😅 . It has a few things done very much not right. OK, so this is what these tests supposed to do:
g = TestSphericalMetric()
M = MetricManifold(Sr, g)
A = Manifolds.get_default_atlas(M)
p = vtype([θ, ϕ])
chart_p = get_chart_index(M, A, p)
B_p = induced_basis(M, A, chart_p, TangentSpace) This is somewhat incorrect as This leads us to reconsider what point (1) does, and thus we should use the chart corresponding to spherical coordinates, not a chart from the default atlas. It's almost as if someone only cared about making these tests pass 😅 . Sorry.
Perhaps the mix between these two charts breaks your new code but I'm not entirely sure. I can at least fix the old tests so that they make sense but you need to give me a few days.
The renaming is certainly fine. The ODE part is very tricky and I think we should be careful here -- it'd very likely take quite a long time to polish it. |
Thanks for the heads up. What do you think about maybe splitting this into two PRs then? I could make a new branch, cherry pick the Renaming commits (or take the lines over), and we do a second one to clear the ODE stuff (maybe on a separate PR maybe on your PR for the ODE in charts)? I would love to have the differentiation part (without ODE) finished to also finish my tutorial on the gradient conversion. I might take a break from the ODE then, though, to first look into the Hessian conversion. But that said, of course there is absolutely no problem if it takes a while :) |
Sure, splitting it into two PRs is a good idea. One PR for renaming, then I can continue my PR to have the geodesics in charts, and then you can work on your approach in another PR.
Sure, the differentiation part should be simple to finish. Hessians (and, probably more importantly, Hessian-vector products) may be an interesting thing to do after that 🙂 . |
The dimensions problems for the solve_ode_exp we will have to consider in another PR (will only do that after the Hessians). |
This PR fixes #428, should also resolve #88. To some extend it might also resolve #42 together with the last PR? At least in the embedding Zygote can be used easily now; for intrinsic methods, there is even in theory still more work to do.
get_vector!
functions missing, that are now required (yields a fewErrorExceptions
whereMethodErrors
were exempted and a few missing implementation errorsThis PR also makes the dependency on FiniteDifferences optional, since we now have enough to choose from.