improve Simpleupdate performances#361
Conversation
Yue-Zhengyuan
left a comment
Yue-Zhengyuan
left a comment
There was a problem hiding this comment.
Thank you for improving SU! Left some initial comments.
| (d, r, c), = _nn_bondrev(sites..., (Nr, Nc)) | ||
| alg.bipartite && r > 1 && continue | ||
| ϵ′ = _su_iter!(state2, gate, env2, sites, alg) | ||
| ϵ′ = _su_iter_gate!(state2, gate, env2, sites[1], sites[2], alg) |
There was a problem hiding this comment.
Here I wanted _su_iter! to be able to handle both AbstractTensorMap{E, S, 2, 2} and length-2 MPO gates, though this is mainly for test purposes to ensure that MPO gates are applied correctly even for fermions. Is it possible to recover this behavior without loss of performance?
There was a problem hiding this comment.
Oh I now see the point of using dispatch. There is a traedoff here between code readibility/efficiency (gate is much faster) and this test feature. Is this something that is currently tested?
[copy from above] In su_iter, we need an if statement to distinguish nearest neighbor from long distance at run time. Therefore my idea was that dispatch does not bring much here, while using different function names makes it explicit and easier to find which function/method is called.
There was a problem hiding this comment.
Is this something that is currently tested?
It is tested with Hubbard model in test/timeevol/cluster_projectors.jl.
gate is much faster
The MPO way can still be optimized. When applying the gate MPO on the first and the last cluster sites, we can also use the trick of reduced bond tensors, which has not been done yet. Even for longer-range (e.g. NNN) bonds, this can still be useful. Separating out two unitary tensors at the two ends of an OBC-MPO will not affect finding the Vidal gauge.
We can try this out in a follow-up PR if you prefer not to do too much here.
There was a problem hiding this comment.
Ok I will revert to dispatch. Keeping this conversation, closing the other relative to this one.
|
There is another recent change that further slowed down SU for 2nd neighbor interaction. After introducing |
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Some small comments, will try and actually look into this in detail next week :)
| gate_axs = alg.purified ? (1:1) : (1:2) | ||
| for gate_ax in gate_axs | ||
| X, a, b, Y = _qr_bond(A, B; gate_ax, positive = true) | ||
| for gate_ax in gate_axs # TODO try to use type stable helper function |
There was a problem hiding this comment.
Will it be better if I always do for gate_ax in 1:2, and manually break after the first loop when alg.purified === true?
| for (site, vertex, open_vaxs) in ((siteA, A, open_vaxs_A), (siteB, B, open_vaxs_B)) | ||
| s′ = (mod1(site[1], Nr), mod1(site[2], Nc)) | ||
| rotated = _bond_rotation(vertex, dir, rev; inv = true) | ||
| state[s′...] = absorb_weight(rotated, env, s′..., open_vaxs; inv = true) |
There was a problem hiding this comment.
The state tensors need normalization, especially when evolving for the ground state; otherwise, the norm will explode.
| rightpermuted = permute(last(Ms), right_invperm) | ||
| state[s′] = absorb_weight(rightpermuted, env, s′, right_vaxs; inv = true) | ||
|
|
||
| for (vertex, s, invperm, vaxs) in zip(Ms[(begin + 1):(end - 1)], sites[(begin + 1):(end - 1)], map(t -> t[3], mids), map(t -> t[2], mids)) |
There was a problem hiding this comment.
@ogauthe Is it really that important to separate out the first and last tensors for type stability because they have 3 instead of 2 open virtual legs?
There was a problem hiding this comment.
Can I mitigate the problem by wrapping contents in the for-loop as a function?
There was a problem hiding this comment.
I think the speedup for the J1-J2 test mainly comes from the improvement in applying nearest neighbor gates. In my test, separating out the first and the last tensors of the cluster offers little gain in performance, especially since we currently only have examples with up to 3-site MPO gates. To reduce redundancy, I'll restore the old approach, but with more things wrapped inside dedicated functions.
There was a problem hiding this comment.
I think indeed a function barrier can mitigate some of it, but it will still give a dynamic dispatch. This might completely be fine though, that by itself doesn't cost too much as long as the type instability doesn't leak out of that region and taint the subsequent code
| local wts | ||
| for gate_ax in 1:2 | ||
| _apply_gatempo!(Ms, gates; gate_ax) | ||
| wts, ϵs = _cluster_truncate!(Ms, truncs) |
There was a problem hiding this comment.
I just realized that I have recently updated _cluster_truncate! in #348, so it can handle MPSs with any number of physical legs (and even different numbers of physical legs at each site). So no longer need to first fuse the physical legs for PEPO.
@ogauthe Do you mind briefly outlining what can be further improved starting from the current PR? |
3 participants
This is a work in progress to improve SimpleUpdate performances.
Context: SimpleUpdate is quite slow, especially for 2nd neighbor interaction. I had cases where it became the bottleneck, above CTMRG. I started working on improving the performances last week. I realized many parts could be ported upstream.
I followed 4 different strategies:
This is a work in progress. It already shows significant speed-up for e.g. finite temperature J1-J2 with SU(2) symmetry. I know it is possible to do much better. I just saw #360 which has overlap for
_get_cluster_truncand I realized it may be relevant to mention my work (objective is not to block any part of #360)