get_vector failing for product manifold #204
Comments
|
Hi, Coll that you are using so many of our packages! Cool to see them in action in such an involved setting – and thanks for your kind words.. I am not yet 100% sure where the error comes from, but I am quite sure its not Manopt but either Manifolds.jl
There we “leave” the realm of what Manopt does (from That said, you should also get the error when you just evaluate your gradient as But then I am a bit lost, since the file your error refers to is not in the current version of
And sure, the error message is a bit long, but that is usual in Julia, one has to learn a bit to read those. here it seems the interaction between product manifold and “generatinng the tangent vector from coordinates in a basis” does not work – but maybe first check which version you are on. PS: The SkewLinearAlgebra (cool package by Simmon!) version should not have much of an effect, this happened somewhere inside building the finite differences (in a Riemannian sense). |
|
Ah! Do not code before first coffee! I misread the error message in For RecursiveArrays, we maybe also need the expert for that for the rescue: @mateuszbaran ! One thing I did check for this is, that your error really alrady appears if your last lines read p = rand(M)
grad_energy(p)so this is not related to |
|
I had a few minutes. So here is at least an M(n)WE reproducing your error, that should be part of the tests once this is fixed using Revise
using Manifolds, ManifoldDiff, RecursiveArrayTools, FiniteDifferences
r_backend = ManifoldDiff.TangentDiffBackend(
ManifoldDiff.FiniteDifferencesBackend()
)
f(p) = 1
grad_f(M,p) = ManifoldDiff.gradient(M, f, p, r_backend)
M = ℂ^3 × PowerManifold(Rotations(6), NestedPowerRepresentation(),3)
p = rand(M)
# causes the same error
grad_f(M,p) |
|
And I think I know the culprit (though not yet the fix)! It‘s the complex manifold. If one looks at the line failing, it checks that we have as many coefficients (the In short the problem is the following:
...and those two cases are mixed up in that place. If you are in a hurry, a quick fix is to phrase your problem on Thanks for pointing this out. |
|
Hi @kellertuer , Thank you very much this swift reply, explanation and suggested quick fix for my use-case. For now I'll work with Many thanks ! |
|
Update : that works like a charm. |
|
I already talked with Mateusz, he will check what we can do, to make the other case work as well, it is mainly this tricky “counting of coefficients” that sometimes haunts us ;) |
|
Purely out of interest, have you had any prior struggles with this ? I can imagine that it's indeed very hard to keep precise track of these types of small details in such an extensive codebase. |
|
Well, we had a bit of trouble with stuff before we noticed there are exactly the two cases I mentioned above (real basis, complex coefficients vs. complex basis, real coefficients), but most of the functions can be solved on a very generic level, because by now both the manifold and the basis have a field type attached. The codebase is okay-ishly extensive, I think it is still fairly structured. |
|
Hi! I've taken a look at this issue and it looks like it is caused by our lack of concept of a real coefficient basis. |
|
Sounds both like a good idea. Thee product basis is then mainly to care for the “mix of coefficient types”? |
|
Well, the product basis is actually for the opposite: the same coefficient type. I think it was a mistake to use the number system argument of AbstractBasis to represent something other than the coefficient type. |
|
I know we discussed this quite a while lenthy, but I usually forget our default and have to look it up. The field in the Hence
So I think both indicators are fine (basis or coefficients); for now its basis, from the last longer discussion we had for this. I feel the current advantage is, that the parameters tells something about the basis actually (and not about the implicitly coefficients that need to be used upfront, which might again also depend on the manifold/tangent space this basis belongs to). |
|
I remember those discussions, and I remember I have never been entirely convinced by this choice. It is fine, as in: we can always find a workaround to every problem, but it seems to me we would have fewer workarounds to do if the field was related to coefficients. And |
|
I remember that as well, and I am not sure why we ended up with that choice – in like, it might also have been me with some arguments for that. If we document it thoroughly both are fine with me. If I remember correctly, currently
If I remember correctly, my argument was, that this way the parameter refers to a property of the basis, which seems more reasonable (to me) than the (implicit) coefficients-upfront-the-basis variant of the parameter. The other way around, Also, if we change it, I fear that would be a bit breaking for any complex manifold.
|
|
Hm, maybe let's discuss it when we meet next time? |
|
We discussed this as Mateusz proposed and have an idea for a nice fix, that will also make some interims values of yours nicer, since they will be real. |
|
This should be fixed in the newest version of Thanks again for spotting this and reporting it :) |
|
Great ! I'll rewrite my code soon to give this a test "in the field" Thanks again for the AMAZING package ! |
|
And as expected the update works perfectly :) ! |
|
Perfect! Thanks again for both reporting this so we could fix it as well as checking back – great to hear that it now works nicely. The old way we had before was not only wrong in a few places, but also misleading, the improved version now seems to work much better then! |
Hi,
Firstly thanks to the devs for this wonderful package ! However, when using it today I ran into some issues which, as far as I understand, comes from the get_vector function failing in the context of product manifolds. However the error message is very vague and not really helpful for an uneducated end-user such as myself.
The full code in question is below. Obviously I have tested if
calc_energy(p, Γ_ref)works well. My apologies in advance if this is just me being stupid and or if this should have been posted as an issue in one of the other packages such asmanifolds.jl.I don't see how this could be relevant but just in case I should mention that I'm running a locally pathced version of
SkewLinearAlgebrawhich contains a custom rrule for pfaffians.The stacktrace is pretty wild, here it is :
The text was updated successfully, but these errors were encountered: