Refactor sweeps interface#143
Merged
Merged
Conversation
added 12 commits
January 30, 2024 14:36
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 1, 2024
mtfishman
reviewed
Mar 2, 2024
mtfishman
reviewed
Mar 2, 2024
added 12 commits
March 14, 2024 16:03
…tional keyword argument. Rename extend to extend_or_truncate and fix bug.
… inside alternating_update, change test to broken.
added 2 commits
March 15, 2024 15:07
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. |
mtfishman
reviewed
Mar 15, 2024
mtfishman
reviewed
Mar 15, 2024
mtfishman
reviewed
Mar 15, 2024
Member
|
Thanks again @b-kloss, this is a big improvement for making the code both simpler and more flexible and customizable! |
Contributor
|
Huge PR! And a lot of work. The code looks so much cleaner now and I'm enthused about the direction this is going. |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This work-in-progress PR implements a more flexible
sweepsinterface foralternating_updatebased 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 otherkwargspassed totdvpwill be moved intoinserter_kwargsfor convenience and continuity with prior interfaces.internal_kwargsis not supposed to be passed totdvp, but instead generated insidetdvp, and in this case contains for example thetime_stepargument.internal_kwargsis also the only NamedTuple which is not strictly checked for passing unexpected keyword arguments.The use of
internal_kwargsallows to make the coupling betweensweep_plangeneration and theupdatersignature looser. Note that the interface is still somewhat verbose: If we wish to pass akwargto bothinserterandupdater, 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_plancould then be made aware of this signature, in order to not addsubstepto theupdater_kwargs. Matt and I discussed this extensively already, discarding this idea as to complicated.Other changes:
Rename(Maybe, let's think about this.)extractertoextract,updatertoupdate,insertertoinsert.sweeps_paramfromalternating_update.sweep_planrelated functionality totree_sweeping.jl.sweep_observerinstead ofregion_observerin tests.treetensornetworks/solvers/*tosolvers, movesolvers/*tosolvers/local_solvers.tree_sweeping.jltosweep_plans.jl.inserter,updater, etc. into NamedTuple, use a Tuple instead.extendorfill_or_pad.sweep_planfunctions vs. inside particular solvers liketdvp.nsitesso 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/extractindependent of directionality of edges, ordering of vertices inregion.alternating_updateandsweep_update, since the latter is nothing but a loop.*_kwargsto observer?ToDo (once design question is settled):
default_sweep_plantransform_operatorToDo 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, whereextractetc. are adapted to handle each element of collection, and the default ones handle only the first.)Improve and generalizeforward_region,reverse_regionandcurrent_orthologic.provide a convenient way tocompose_updaters(...)(Re)Implement good default printers/observers at least for DMRG, TDVPImplement noise-term for DMRG or alternative.