Remove t argument from alternating_update and improve keyword argument handling

[emstoudenmire]

Improvements Made in this PR

• Removes t argument from alternating_update in favor of using the tdvp interface to process time evolution related inputs. (alternating_update is now only given nsweeps, basically, plus some other time related keywords, some of which are slated for removal, others of which are just passed down to the ultimate solver).
• This PR makes significant improvements to keyword argument handling.
• The _compute_nsweeps function has been moved into tdvp.jl and is only used by the tdvp interface
• The solver no longer takes a t argument. The only solver which uses time concepts now is the tdvp solver, which gets passed a time_step keyword arg (this value includes all of the information about sub time steps, the forward or reverse sign, etc.)

Planned Improvements in Future PRs (Can discuss moving them into this PR if it's a good idea)

• No longer handling the current_time computation and printing directly in the alternating_update, update_step etc. code directly (would let us remove most of the remaining time related keyword args, like time_start, or needing to pass current_time up and down the function stack)
• Finishing factoring out the logic around the TDVPOrder type so that it only goes in tdvp.jl and is no longer involved in algorithms like dmrg, linsolve, etc.
• Figuring out if we want to factor out the forward/reverse step logic into tdvp.jl also, or whether it's ok for some of that to persist inside alternating_update
• Further improvements to keyword arg handling
• Finish simplying solvers to only be a single function, rather than a function that returns another function (unless neccessary)

[codecov-commenter]

Codecov Report

Merging #67 (3960493) into main (536faaa) will decrease coverage by 0.10%.
The diff coverage is 87.09%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #67      +/-   ##
==========================================
- Coverage   76.82%   76.72%   -0.10%     
==========================================
  Files          60       60              
  Lines        3115     3072      -43     
==========================================
- Hits         2393     2357      -36     
+ Misses        722      715       -7     

Impacted Files	Coverage Δ
src/treetensornetworks/solvers/linsolve.jl	60.00% <0.00%> (+10.00%)	⬆️
src/treetensornetworks/solvers/tdvporder.jl	33.33% <25.00%> (ø)
src/treetensornetworks/solvers/tdvp.jl	73.33% <81.25%> (+4.91%)	⬆️
src/treetensornetworks/solvers/update_step.jl	83.90% <95.00%> (-1.67%)	⬇️
...c/treetensornetworks/solvers/alternating_update.jl	80.70% <100.00%> (+1.15%)	⬆️
src/treetensornetworks/solvers/contract.jl	91.66% <100.00%> (-0.93%)	⬇️
src/treetensornetworks/solvers/dmrg.jl	100.00% <100.00%> (ø)
src/treetensornetworks/solvers/dmrg_x.jl	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[emstoudenmire]

What do you think of the keyword name? time versus other ones above.

[mtfishman]

Also, what do you think about using the keyword argument nsteps instead of nsweeps in the tdvp interface? Then nsteps, t, and time_step could get processed into nsweeps which gets input into alternating_update.

[emstoudenmire]

I think making the interface take nsteps is probably the right way to go. I'll look into it more!

It will be interesting how much we can "orthogonalize" the alternating_update code away from TDVP-specific concepts. Right now, there is still a lot of the plumbing that e.g. relies on a TDVPOrder object or the concept of sub time steps. But I think I can further refactor the code to have things like perhaps two versions of update_step (which is a short function anyway), one which deals with TDVPOrder concepts and another which just subs in defaults for more normal dmrg-like algorithms. Either that, or we could elevate the TDVPOrder object to a more general pattern, like UpdatePattern or some other concept.

Luckily I don't think local_update or _insert_tensor depend on TDVP concept in as much of an essential way.

[mtfishman]

Yes, I think we should push the TDVPOrder to the top level tdvp wrapper, and then process that in a more general functionality for determining the nweeps and time steps for a given t, TDVPOrder, time_step, and nsteps.

(Not saying we need to do that in this PR, that sounds like a more involved change so maybe is better to do in future work.)

[emstoudenmire]

The time-dependent Hamiltonian tests aren't passing. I'll work on it –

[emstoudenmire]

Looks like tests are passing now.

This PR overall is just a stepping stone and some initial cleanup to get the code to where a refactor is easier to do, and to check some boxes on the associated issue #60. Definitely a lot more work to do on this code! I think the rest of the #60 boxes will be easier to check when I put in the design we discussed about passing an array of sweeping steps and "payload" data for the solver to consume.

[emstoudenmire]

I'm having trouble getting the tests to pass, and it seems to unfortunately be something not obviously related to this PR. I see an error about part of a graph related code trying to access an empty tuple at [1] but I'm not otherwise sure what is really behind this.

I thought maybe it was a bug introduced by another recent PR, possibly, but when I run some of the ITensorNetworks tests on my own machine on the main branch they pass.

This PR is ready to be merged in from my end, if you agree, and of course once tests pass. Meanwhile I am working on some of the other improvements to this code on a new branch starting from this one, to be sent in another PR.

[mtfishman]

Looks like tests are passing, I'll merge. Thanks, nice to see some of this code getting cleaned up!

EDIT: I hadn't reloaded the page and so was seeing old test results that had been passing, guess they are failing now.

[mtfishman]

I think the failures are related to ITensor/DataGraphs.jl#17, I'll look into it.

[mtfishman]

Alright, with the fix from ITensor/DataGraphs.jl#18 this now passes, I'll merge this.

[emstoudenmire]

Thanks! The code is already simplifying a lot just from these changes and I think it can be simplified even more.