-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor sweeps interface #143
Conversation
…tional keyword argument. Rename extend to extend_or_truncate and fix bug.
… inside alternating_update, change test to broken.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #143 +/- ##
==========================================
- Coverage 73.61% 73.60% -0.02%
==========================================
Files 71 74 +3
Lines 4146 4239 +93
==========================================
+ Hits 3052 3120 +68
- Misses 1094 1119 +25 ☔ View full report in Codecov by Sentry. |
Thanks again @b-kloss, this is a big improvement for making the code both simpler and more flexible and customizable! |
Huge PR! And a lot of work. The code looks so much cleaner now and I'm enthused about the direction this is going. |
This work-in-progress PR implements a more flexible
sweeps
interface foralternating_update
based algorithms.Currently, a proof-of-principle is implemented for
tdvp
, while other algorithms are broken. The purpose of this draft is to discuss the overall structure of the code, not the details of the implementation of specific functions.The interface is based around passing a list of length
nsweeps
, each of which contains fourNamedTuples
:extracter_kwargs
,updater_kwargs
,inserter_kwargs
,internal_kwargs
. The latter hold the epynomous functions as well as their specified keyword args and are passed as kwargs totdvp
, with the exception ofinternal_kwargs
. Any otherkwargs
passed totdvp
will be moved intoinserter_kwargs
for convenience and continuity with prior interfaces.internal_kwargs
is not supposed to be passed totdvp
, but instead generated insidetdvp
, and in this case contains for example thetime_step
argument.internal_kwargs
is also the only NamedTuple which is not strictly checked for passing unexpected keyword arguments.The use of
internal_kwargs
allows to make the coupling betweensweep_plan
generation and theupdater
signature looser. Note that the interface is still somewhat verbose: If we wish to pass akwarg
to bothinserter
andupdater
, we need to list it twice in the call totdvp
.A more complicated design would be to group the function (e.g.
updater
) and a collection of symbols (i.e. keys ofkwargs
) it can digest. For example for the default TDVP behaviour, we would use something like(exponentiate,[:time_step])
.tdvp_sweeps_plan
could then be made aware of this signature, in order to not addsubstep
to theupdater_kwargs
. Matt and I discussed this extensively already, discarding this idea as to complicated.Other changes:
Rename(Maybe, let's think about this.)extracter
toextract
,updater
toupdate
,inserter
toinsert
.sweeps_param
fromalternating_update
.sweep_plan
related functionality totree_sweeping.jl
.sweep_observer
instead ofregion_observer
in tests.treetensornetworks/solvers/*
tosolvers
, movesolvers/*
tosolvers/local_solvers
.tree_sweeping.jl
tosweep_plans.jl
.inserter
,updater
, etc. into NamedTuple, use a Tuple instead.extend
orfill_or_pad
.sweep_plan
functions vs. inside particular solvers liketdvp
.nsites
so they can be functions of the sweep.process_kwargs_for_sweeps
.sweep_plans/
,alternating_update/
,region_update/
.region_update
, make a single named tuple for keyword arguments being passed toupdate!
andregion_printer
.region_printer
,sweep_printer
,region_observer!
,sweep_observer!
are passed.make_regions
, defineforward_regions(nsite, graph)
such thatforward_regions(2, path_graph(10)) = [(1, 2), (2, 3), (3, 4), ..., (9, 10)]
andinsert_region_intersections(forward_regions(2, path_graph(10))) = [(1, 2), (2,), (2, 3), (3,), (3, 4), ...(9,), (9, 10)]
.insert
/extract
independent of directionality of edges, ordering of vertices inregion
.alternating_update
andsweep_update
, since the latter is nothing but a loop.*_kwargs
to observer?ToDo (once design question is settled):
default_sweep_plan
transform_operator
ToDo in future PRs (these haven't been implemented, but are being tracked in an external document now):
Implement passing more than one cache to alternating update (e.g. as a collection, whereextract
etc. are adapted to handle each element of collection, and the default ones handle only the first.)Improve and generalizeforward_region
,reverse_region
andcurrent_ortho
logic.provide a convenient way tocompose_updaters(...)
(Re)Implement good default printers/observers at least for DMRG, TDVPImplement noise-term for DMRG or alternative.