Skip to content
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

Merged
merged 71 commits into from
Mar 18, 2024
Merged

Refactor sweeps interface #143

merged 71 commits into from
Mar 18, 2024

Conversation

b-kloss
Copy link
Contributor

@b-kloss b-kloss commented Feb 29, 2024

This work-in-progress PR implements a more flexible sweeps interface for alternating_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 four NamedTuples: 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 to tdvp, with the exception of internal_kwargs. Any other kwargs passed to tdvp will be moved into inserter_kwargs for convenience and continuity with prior interfaces. internal_kwargs is not supposed to be passed to tdvp, but instead generated inside tdvp, and in this case contains for example the time_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 between sweep_plan generation and the updater signature looser. Note that the interface is still somewhat verbose: If we wish to pass a kwarg to both inserter and updater, we need to list it twice in the call to tdvp.

A more complicated design would be to group the function (e.g. updater) and a collection of symbols (i.e. keys of kwargs) 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 add substep to the updater_kwargs. Matt and I discussed this extensively already, discarding this idea as to complicated.

Other changes:

  • Rename extracter to extract, updater to update, inserter to insert. (Maybe, let's think about this.)
  • Removed sweeps_param from alternating_update.
  • Move sweep_plan related functionality to tree_sweeping.jl.
  • Compute observables with sweep_observer instead of region_observer in tests.
  • Move treetensornetworks/solvers/* to solvers, move solvers/* to solvers/local_solvers.
  • Rename tree_sweeping.jl to sweep_plans.jl.
  • Instead of processing inserter, updater, etc. into NamedTuple, use a Tuple instead.
  • Define a function extend or fill_or_pad.
  • Put more logic into sweep_plan functions vs. inside particular solvers like tdvp.
  • Extend kwargs like nsites so they can be functions of the sweep.
  • Rename and refactor process_kwargs_for_sweeps.
  • New directory structure: sweep_plans/, alternating_update/, region_update/.
  • Inside region_update, make a single named tuple for keyword arguments being passed to update! and region_printer.
  • Update how region_printer, sweep_printer, region_observer!, sweep_observer! are passed.
  • Refactor make_regions, define forward_regions(nsite, graph) such that forward_regions(2, path_graph(10)) = [(1, 2), (2, 3), (3, 4), ..., (9, 10)] and insert_region_intersections(forward_regions(2, path_graph(10))) = [(1, 2), (2,), (2, 3), (3,), (3, 4), ...(9,), (9, 10)].
  • Make tdvp compatible with various signatures for specifying timestep, including list of timesteps, which gets expanded.
  • define defaults, in separate, package-wide file?
  • Make insert/extract independent of directionality of edges, ordering of vertices in region.
  • Combine alternating_update and sweep_update, since the latter is nothing but a loop.
  • handle passing of args to observer (and to lesser extent printer) in a similar manner, or just pass all *_kwargs to observer?

ToDo (once design question is settled):

  • adapt other solvers/default_sweep_plan
  • Add tests for time-step specification
  • Add tests for 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, where extract etc. are adapted to handle each element of collection, and the default ones handle only the first.)
  • Improve and generalize forward_region, reverse_region and current_ortho logic.
  • provide a convenient way to compose_updaters(...)
  • (Re)Implement good default printers/observers at least for DMRG, TDVP
  • Implement noise-term for DMRG or alternative.

@b-kloss b-kloss marked this pull request as ready for review March 15, 2024 18:58
@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 82.27848% with 70 lines in your changes are missing coverage. Please review.

Project coverage is 73.60%. Comparing base (65ca518) to head (d2a306c).
Report is 1 commits behind head on main.

Files Patch % Lines
src/solvers/defaults.jl 15.15% 28 Missing ⚠️
src/solvers/tdvp.jl 74.00% 13 Missing ⚠️
...c/solvers/alternating_update/alternating_update.jl 82.60% 8 Missing ⚠️
src/solvers/contract.jl 80.95% 8 Missing ⚠️
src/solvers/local_solvers/linsolve.jl 0.00% 5 Missing ⚠️
src/utils.jl 86.66% 4 Missing ⚠️
src/solvers/alternating_update/region_update.jl 95.12% 2 Missing ⚠️
src/solvers/linsolve.jl 60.00% 2 Missing ⚠️

❗ 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.
📢 Have feedback on the report? Share it here.

@mtfishman
Copy link
Member

Thanks again @b-kloss, this is a big improvement for making the code both simpler and more flexible and customizable!

@mtfishman mtfishman merged commit a4f3592 into ITensor:main Mar 18, 2024
7 of 11 checks passed
@emstoudenmire
Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants