Closed Gertian closed 1 month ago
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
[27] grad_energy(p::ArrayPartition{ComplexF64, Tuple{Vector{ComplexF64}, Vector{Matrix{ComplexF64}}}}) @ Main ~/Documents/Projects/gaussian_MPS/superposed_dense/superposed_dense.jl:98
There we “leave” the realm of what Manopt does (from [31]
and [30]
) you can see this is the very first call to the gradient evaluation. From there it seems that it is maybe not ManifoldsDiff
because we get through that nicely I think,
but we are stuck in the RecursiveArrays extension of Manifolds.jl
.
That said, you should also get the error when you just evaluate your gradient as grad_energy(p)
for your start point rand(M)
.
Not that your gradient has the manifold hardcoded saved in itself, since it is not a parameter of the gradient; we usually use grad_f(M,p)
, but that should not be the cause of the error, just a matter of style.
But then I am a bit lost, since the file your error refers to is not in the current version of Manifolds.jl
. Which version are you running on? I would recommend upgrading to 0.10.x, because this might have been a bug we already fixed.
grad_energy(p) = ManifoldDiff.gradient(M, p->calc_energy(p, Γ_ref), p, r_backend)
, your function f
(the second argument) does not have the manifold as first argument. Are you sure this is right?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 [1]
the error stems from ManifoldsBase even, see the code line
https://github.com/JuliaManifolds/ManifoldsBase.jl/blob/30d587bcef68a35146ef7a86c31e3182b5e29c19/ext/ManifoldsBaseRecursiveArrayToolsExt/ProductManifoldRecursiveArrayToolsExt.jl#L102 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 Manopt.jl
indeed and I‘ll transfer this to where the error appears – ManifoldsBase! I am sure we can narrow that down and find the culprit and fix it :)
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. (Sorry for so many posts – I had a bit more time than I initially thought)
If one looks at the line failing, it checks that we have as many coefficients (the Xⁱ
) as we have dimensions in the manifold.
In your case we have 48 coefficients but a dimension 51; but also that the coefficients are complex (actually all of them), which sounds fair for the first 3, but then we also just need 3 (complex) coeffcients for th 6-dimensional manifold ℂ^3
not 6.
So we have to check how to best resolve this. Some areas of Manifolds.jl / ManifoldsBAse.jl were maybe not yet fully tested with complex it seems – so great that you use that now and we can fix this.
In short the problem is the following: For the complex numbers you can provide two kinds of basis:
...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 $\mathbb R^{2n}$ instead since you seem to have the corresponding rotation matrices already. Otherwise wait for us to find a good fix here.
Thanks for pointing this out.
Hi @kellertuer ,
Thank you very much this swift reply, explanation and suggested quick fix for my use-case. I'm super happy that I'll be able to continue with this project this quickly !
For now I'll work with $$\mathbf{R}^{2n}$$ and report back if that's working. Off course, if you later want me to try the full code again with $$\mathbf{R}^{n}$$ I'll be happy to do so.
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.
I hope we got it right for most manifolds by now – maybe besides Product – or maybe in a few ones of Euclidan
we were not careful enough. Sometimes the simple manifolds are the meanest, because by thinking they are super simple one might miss exactly a case like you stumble into now.
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. DefaultOrthonormalBasis()
can either be real-coefficient or complex-coefficient, depending on the manifold. @kellertuer I guess the easiest solution would be to introduce a function for getting the (default-ish) real coefficient basis of a manifold? And that we would also need a ProductBasis
for M
.
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 AbstractBasis
refers to the type used in the basis elements, not their coefficients upfront.
Hence
Real
means we have a real basis; so we have less basis vectors than the dimension on complex manifolds and that means we need complex coefficients in the sumComplex
mean we have complex values in the basis, then we have a (complex) basis but with the same number of elements as the dimension and hence only need real coefficients.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 Real
doesn't really mean we have a real basis. You can have complex basis vectors in a complex coefficient basis. The only fundamental part is the field over which the vector space (such as tangent space) is defined. And that field corresponds to coefficients.
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
Real
would indeed mean the arrays of the basis are real-valued (and on complex that means the coefficients upfront the basis elements are complex). For real manifolds, this is the only good choice for a basisIf 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, Real
would indicate for a complex manifold, that we have a complex basis, isn't that a bis misleading that Basis{Real}
then refers to a basis with complex tangent vectors?
Also, if we change it, I fear that would be a bit breaking for any complex manifold. I can check the next few days what we have to fix for the bug reported here.
ProductBasis
to case a bit for the mix of these? We could maybe even check how to best do a product basis where parts of the coefficients are real, and parts are complex – or should that always lead to complex coefficients but we only pass down the real part of this to real manifolds?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 ManifoldsBase.jl
(0.15.17) and with https://github.com/JuliaManifolds/Manifolds.jl/pull/754 also throughout the rest of the ecosystem.
Thanks again for spotting this and reporting it :)
Great ! Thanks for looking into this so quickly.
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
SkewLinearAlgebra
which contains a custom rrule for pfaffians.The stacktrace is pretty wild, here it is :