Refactor interfaces built around alternating update #121
Conversation
…amiltonian to solver by reference, passes more information about sweep to solver.
… solver interface.
|
Thanks for getting started with this. Could you format? It makes it easier to review the PR. |
|
|
| which_sweep, | ||
| region_updates, | ||
| which_region_update, | ||
| region_kwargs, |
There was a problem hiding this comment.
I think region_update_kwargs sounds better to me.
There was a problem hiding this comment.
Though the same discussion on exponentiate_updater applies here, I think we should discuss how these are being passed and maybe merge them with updater_kwargs.
There was a problem hiding this comment.
Since this will be obsolete soon, I am in favor of sticking with region_kwargs.
| region_kwargs, | ||
| updater_kwargs, |
There was a problem hiding this comment.
Is the plan to combine these into updater_kwargs?
There was a problem hiding this comment.
updater_kwargs are updater specific kwargs, while region_args are among the things that we expose to all updaters. in principle, we can nest the region_args into updater_kwargs in the call to region_update but I am not sure if that's preferable.
There was a problem hiding this comment.
Hmm ok, I just have found it hard to keep track of the logic of why certain keyword arguments are bundled in certain ways, how they will be used, etc.
For example, from the perspective of this function, the only argument I can see that is being used here from region_kwargs is time_step, which I don't think is really any different conceptually from the arguments being passed in updater_kwargs (it's just another thing being used by the solver/updater). So it makes sense to me to just bundle those together in one flat NamedTuple called updater_kwargs.
There was a problem hiding this comment.
As discussed, these will eventually be bundled together in an upcoming PR.
| function dmrg_sweep( | ||
| nsite::Int, graph::AbstractGraph; root_vertex=default_root_vertex(graph) | ||
| ) | ||
| return tdvp_sweep(2, nsite, Inf, graph; root_vertex, reverse_step=false) |
There was a problem hiding this comment.
| return tdvp_sweep(2, nsite, Inf, graph; root_vertex, reverse_step=false) | |
| order = 2 | |
| time_step = Inf | |
| return tdvp_sweep(order, nsite, time_step, graph; root_vertex, reverse_step=false) |
so we remember what 2 and Inf mean.
There was a problem hiding this comment.
Also, I'm not sure how I feel about calling this dmrg_sweep since it could be used by other solvers like linsolve. I'm also not a huge fan of the function name tdvp_sweep for the same reason.
Maybe we can rethink this API a bit, for example rename the current tdvp_sweep to default_sweep_plan and then just call that with different inputs from within the different solvers. I think a good design for that would be to choose default values that "just work" for DMRG, including defaulting to order=2, and then provide optional inputs like reverse_step=true and time_step for use with TDVP. That may mean moving some of the inputs like order and time step to keyword arguments, which seems like a better design anyway.
There was a problem hiding this comment.
Yes, and this is what the default_sweep_regions function was trying to be. That is, a sort of "typical" sweep plan that most algorithms (dmrg, linsolve, etc.) could use. We could definitely think about moving that function out of the update_step.jl file but the intent there was for it to be a pretty generic default.
There was a problem hiding this comment.
I see, I forgot about default_sweep_regions, I'm not sure what the fate of that is in the current PR since a lot of things are getting changed in this PR. But it seems like default_sweep_regions, tdvp_sweep, and dmrg_sweep should all get consolidated into a single function, which tentatively we are thinking of calling something like default_sweep_plan, default_region_updates, or default_region_update_plans (since it contains both the regions that will be updated but also information about how to do the update), which I think should just get called directly by the different solvers like dmrg, tdvp, linsolve, etc. and passed to alternating_update.
Opinions on what to name that function would be welcome. We want to come up with a good name for that function and what it outputs which we can then use consistently throughout the rest of the code. The list output by tdvp_sweep/dmrg_sweep is called region_updates in the current version of this PR but we are discussing alternative names, which hopefully will be consistent with the name of the new function.
There was a problem hiding this comment.
I incorporated the suggestion, and the sweep_regions constructor is planend to be refactored in an upcoming PR
That's too bad. If you think it is a bug with the formatter (i.e. the code you wrote is correct Julia syntax and runs properly, but the formatter errors on it), you can try to track which line of code is giving the formatter issues and then tell the formatter to skip those lines of code with https://domluna.github.io/JuliaFormatter.jl/stable/skipping_formatting/#Skipping-Formatting. Then ideally we could make a minimal example of what is giving the formatter issues and raise an issue.
I would generally bias towards naming the keyword arguments after the keyword argument names of the solver, so then we can just point to the documentation of those solvers for a list of supported keyword arguments. |
|
Missed the time-dependent tdvp tests. |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
+ Coverage 71.37% 72.55% +1.17%
==========================================
Files 67 71 +4
Lines 4098 4055 -43
==========================================
+ Hits 2925 2942 +17
+ Misses 1173 1113 -60 ☔ View full report in Codecov by Sentry. |
|
Looks good, thanks! |
This PR aims to make the solver interface more flexible without changing the existing structure substantially. Examples of target use cases:
a) A call to a local subspace expansion preceding every local solve. This would be achieved by defining a solver that calls a local expander routine before e.g.
exponentiate(both of which follow the solver function and return signature).b) A call to a global/sweep subspace expansion preceding every n sweeps of TDVP with
exponentiateas the local solver. This would be accomplished by defining a custom sweeps function that assembles the above pattern, passing the solver as astep_kwarg.psi::TTN, and the projected Hamiltonian,PH::AbstractProjTTN, as a keyword arg by reference all the way to thesolverfunctions which is called bylocal_update. This allows the solver function to modify the latter out-of-place, a functionality needed e.g. for subspace expansions accessed as a solver.a) Future goal: Pass the solver function as
step_kwarg, s.t. the decision about what gets done locally is accessible by modifying the sweeps pattern (which is defined at the same level as the solver).b) Currently: Implement all of the logic of choosing what solver to use locally inside the solver function (which should have the necessary information about the position in the sweep etc).
dmrgsolver interface and tests.dmrg-xsolver inteface and tests. Also implement linsolve?noiseas updater (for full DMRG functionality, the alternative being subspace expansion (or both) )Naming updates:
region_kwargstoregion_update_kwargs(or combine withupdater_kwargs?).psi_ref!tostate!andPH_ref!toprojected_operator!.psi0toinit_state,psitostate,Htooperator,PHtoprojected_operator.Open questions
extract_local_tensorwhen region is an edge (single-site TDVP). Subspace expansion requires truncation at this step, but could be done inside solver, at the cost of performing SVD of a single-vertex-tensor twice.extract/insert_local_tensorusingregionand neighbouringregion?