some patches #96
some patches #96
Conversation
| @@ -51,7 +51,7 @@ function make_eigsolve_pullback(config, f, fᴴ, x₀, howmany, which, alg_prima | |||
| n_vals = isnothing(_n_vals) ? 0 : _n_vals | |||
| n_vecs = isnothing(_n_vecs) ? 0 : _n_vecs | |||
| n = max(n_vals, n_vecs) | |||
| if n < length(vals) && vals[n + 1] == conj(vals[n]) | |||
| if n != 0 && n < length(vals) && vals[n + 1] == conj(vals[n]) | |||
There was a problem hiding this comment.
Why does the n==0 case need to be excluded? Does it happen in a VUMPS context that one of the eigenvalue problems has two complex conjugated eigenvalues of largest magnitude?
There was a problem hiding this comment.
Yes, there are always degenerate eigenvalues when initializing a large unit cell PEPS randomly. Maybe there is a better trick to avoid this.
There was a problem hiding this comment.
This might be an artifact of the way you supply your operators: if I understood correctly you always transfer over a single site, and take all the collection of all tensors as the "eigenvector", which indeed trivially has eigenvalues with phases 2 * pi * k / L because you can cycle through the unitcell without these affecting the final result, leading to conjugate eigenvalues whenever the unitcell is even(?).
I don't know the answer to this: is it in this case fair to just consider the one you obtain first? It seems like this might be sensitive to initial conditions etc, and I am not sure if this rrule (mathematically) is correct.
There was a problem hiding this comment.
I clearly misread this, the n==0 case just leads to the indexing error with vals[n] and does definitely need to be excluded. May I suggest to use simply
if 0 < n < length(vals) && vals[n + 1] == conj(vals[n])
Also, using the exact equality as a way to "hope" that this will only be satisfied in the case where the original schursolve was performed in real arithmetic is perhaps a bit of a hack. However, at the level of eigsolve, I don't think it can still be detected whether the original problem represented a real-value linear operator or a complex-valued one, so I don't have a better suggestion for that.
| @@ -11,5 +11,6 @@ include("utilities.jl") | |||
| include("linsolve.jl") | |||
| include("eigsolve.jl") | |||
| include("svdsolve.jl") | |||
| include("patch.jl") | |||
There was a problem hiding this comment.
Thanks for the patch. I think you can just directly put the contents of "patch.jl" directly into "KrylovKitChainRulesCoreExt.jl"
There was a problem hiding this comment.
Perhaps rename it to constructor.jl for potential additions in the future.
There was a problem hiding this comment.
To be honest, it might be better to just deprecate RecursiveVec to be a regular Vector. Since the VectorInterface overhaul, both Vectors and Tuples of whatever you would feed it should just work, so I would rather phase out this type, instead of increasing its capabilities. Could you try if that also solves your use-case?
There was a problem hiding this comment.
Yes, I agree with you. Indeed, I didn't use RecursiveVec in the current AD-VUMPS. I added it because I tried to make MPSKit.jl differentiable at first, and it's needed.
There was a problem hiding this comment.
I think we can remove that in MPSKit.jl, and add the deprecation here. Then, you can just leave out the constructor.jl file and everything should be good to go.
There was a problem hiding this comment.
I can also do this in the update. But if we deprecate RecursiveVec at this point, then this becomes KrylovKit 0.9 instead of 0.8.2. That is fine with me, but that this trigger more necessary updates elsewhere?
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
I think I like it better with an empty line here :)
| @@ -51,7 +51,7 @@ function make_eigsolve_pullback(config, f, fᴴ, x₀, howmany, which, alg_prima | |||
| n_vals = isnothing(_n_vals) ? 0 : _n_vals | |||
| n_vecs = isnothing(_n_vecs) ? 0 : _n_vecs | |||
| n = max(n_vals, n_vecs) | |||
| if n < length(vals) && vals[n + 1] == conj(vals[n]) | |||
| if n != 0 && n < length(vals) && vals[n + 1] == conj(vals[n]) | |||
There was a problem hiding this comment.
This might be an artifact of the way you supply your operators: if I understood correctly you always transfer over a single site, and take all the collection of all tensors as the "eigenvector", which indeed trivially has eigenvalues with phases 2 * pi * k / L because you can cycle through the unitcell without these affecting the final result, leading to conjugate eigenvalues whenever the unitcell is even(?).
I don't know the answer to this: is it in this case fair to just consider the one you obtain first? It seems like this might be sensitive to initial conditions etc, and I am not sure if this rrule (mathematically) is correct.
XingyuZhang2018
force-pushed
the
AD-VUMPS
branch
from
7003eb6 to
a700c2d
Compare
|
I revised the commit. Dealing with |
|
This looks great; I will merge as soon as CI finishes. I have another small update ready, and then we can tag a new release. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #96 +/- ##
==========================================
- Coverage 84.62% 84.53% -0.10%
==========================================
Files 31 31
Lines 3318 3323 +5
==========================================
+ Hits 2808 2809 +1
- Misses 510 514 +4 ☔ View full report in Codecov by Sentry. |
I have identified certain patches that I deem necessary when implementing AD-VUMPS.