Refactor Identity to not allocate memory if not necessary#401
Refactor Identity to not allocate memory if not necessary#401kellertuer merged 97 commits intomasterfrom
Conversation
…dentity was something completely different to the Base identity method)
|
I got a little stuck on one point that should be similar to the allocation of
does not have an My idea is as follows: In a few places we have to have a |
|
I don't think we should support |
|
There is a lot a lot of places, where currently |
|
OK, I just confused it with |
|
Yes, sorry, that that was confusing, I had no better idea for a name for that function, it would maybe also be a |
|
Further I refactored edit: So there is one challenge we currently have:
While working on this I noticed that we could also refactor the |
|
There is still a lot of errors and failing tests, but we should first decide where to keep identity before we continue. the overloaded operators are done, compose and apply should work, but for example the semidirect product group is still missing some features. |
I don't like it, one-argument
I think there would be too many ambiguities with this. If anything, we could refactor
I'll take a look at it today. |
I saw that, too, but I did not have a better idea for a name. The only idea I had was edit; We could take
We would have
and similar for the log. I think there should not be any ambiguity. Which ambiguities do you see? For me the name
Great! I can surely help once we fixed, where to use |
We have |
I would not define this for arbitrary manifolds, just
(and the other 3 analogously), or even with |
But how does it remove the ambiguity for |
|
ah, but that is really only for the circle group, right? Hm. |
|
It seems that we have some problems with plot recipe testing. VisualRegressionTest seems to be no longer maintained: JuliaPlots/VisualRegressionTests.jl#32 . |
|
hm, without those we lose quite some coverage, that would be sad. |
|
Also documentation fails, maybe we won't be able to continue using that very old version of Plots.jl? Do out plots still fail on newer Plots.jl? |
|
I was surprised by that too, but forgot that (and why) we fixed plots that way, but the newest version also throws errors, and I have not enough time just now to check why. So we either have to stay with that version or delete the recipes for now. Ah it really just seems to be the regression test; the rest works as it seems. |
…ecipes line, since plots are still called.
|
Meh. Now we are lost in some Unsatifyability infinite loop – locally the tests run fine, but somehow deleting VisualRegressionTests here for now introduced a laaaaaaaarge unsatisfiability that I do not even understand. I had hoped that deleting a package would reduce incompatibilities not raise new ones. |
|
That issue is much easier to understand when printed without word wrap. It turns out that 1.6.12 was the last version of Plots.jl that supports Julia 1.4 so we are either stuck with it and its problems or drop Julia 1.4. Julia 1.5 is still fine. |
Ah that's why there was a 1.6 in there, will try to add that again then. Yeah somehow I dod not get a version that was wrapped, also not when copying the text and I was not sure which package caused all this. |
|
I noticed there is also a second issue: It's weird, it worked before -- incompatibility seems to be caused by ChainRulesCore. |
Co-authored-by: Mateusz Baran <mateuszbaran89@gmail.com>
This PR seems to be cursed somehow. I again do not understand where this is now coming from just because we loosened the compat on plots or deleted the visual regression tests...?! Edit maybe an artefact somehow? It seems to be gone now and we even have enough code coverage 🥇 |
|
I don't understand it but if CI thinks it's fine then I'm not going to argue 😄 . |
|
Hi, I was reading changes for v0.6 and found this thread. Perhaps the issue between local and CI test failures has to do with a known cached registry issue. Cache does not update for upstream immediately if there are large file size changes (resulting in a delay for DiffEqBase / ChainRules in this case). I previously had a similar issue which took me forever to figure out (leading to Registry 16777, a very long thread), and after parsing through it was able to fix with: |
|
Thanks :) Well the issue went away (and even nightly ran fine on master after merging), so it resolved itself – but thanks for the comment, that might indeed habe been the case. My main problem was then maybe, that this PR was far longer and far more work than I thought. |
|
more than a thousand lines of changes to update the description of the zero/nothing/identity element... :-) We're looking at something similar, for the inference code to go beyond Lie groups (requiring the floating reference point rather than assuming identity). It's also becoming a large influence on code design our side... I'm likely using identity way too much. PS, we're pushing to finish IncrementalInference.jl v0.25 which will be fully converted to using/following Manifolds.jl! And, a few downstream packages too. |
|
Well, the reason was simple: The initial design of identity was to avoid allocations – otherwise one could just the array/AbstractManifoldPoint instead. Then with further features the identity stored the point internally. This PR refactored that, so that it was again a type without allocations to the PS: Cool! Looking forward to seeing that! |
|
Okay, that sounds good! I'm having a little unexpected difficulty around using identity as we had been doing before. Trying to debug... What is the correct way to do this for M = SpecialEuclidean(2)
G = Identity(M)
julia> Manifolds.identity_element(G)
ERROR: MethodError: no method matching identity_element(::Identity{Manifolds.SemidirectProductOperation{RotationAction{TranslationGroup{Tuple{2}, ℝ}, SpecialOrthogonal{2}, LeftAction}}})
Closest candidates are:
identity_element(::ProductGroup{𝔽, T} where {𝔽, T}) at /home/dehann/.julia/packages/Manifolds/EY0G1/src/groups/product_group.jl:37
identity_element(::CircleGroup) at /home/dehann/.julia/packages/Manifolds/EY0G1/src/groups/circle_group.jl:27
identity_element(::CircleGroup, ::Number) at /home/dehann/.julia/packages/Manifolds/EY0G1/src/groups/circle_group.jl:28
.. |
|
Could you open an issue for this? The PR is already merged and finished. Then I will take a look, I hope we have not missed an |
This PR is a WIP to reduce
Identity(see #378) to not contain its point anymore. Here are a few notes on my ideaIdentity{G}is still parametrised by the group it belongs toidentity!, ìndentityandmake_identity` are obsoleteget_point(e::Identity{G})(orget_point(g::G, Identity{G})should be implemented to return the (array) point that represents the identityget_vectorandget_coordinatesbut maybe they can be reintroduced