-
Notifications
You must be signed in to change notification settings - Fork 23
Changes to the iterator interface #255
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
Changes to the iterator interface #255
Conversation
RegionPlan is ommited as this is just vector of kwargs whos type is unimportant
…stract type Other changes: - Both `sweep_callback` and `region_callback` in `sweep_solve` now take only one positional argument, the sweep iterator. - Iterating `SweepIterator` now automatically performs the RegionIteration - Added an 'adapter' `PauseAfterIncrement` that allows `SweepIterator` to be iterated without performing region iteration - `RegionIteration` now tracks the current sweep number - Replaced some function calls with explict calls to constructors to make it clear when new iterators are being constructed (instead of returned from a field etc). Note, AbstractNetworkIterator interface requires some documentation.
|
…me but returning a tuple (region, kwargs) at each step
… true during the iteration
…`prev`/`next` naming convention
To be simplified...
This removes the function `region_step`. Default kwargs for these functions (none currently) are now splatted in using the `default_kwargs` function. Remove `sweep` kwargs as this can be obtained from the `RegionIterator`.
Includes the code change `(region, kwargs)` to `region => kwargs` for readability, but I also think the `Pair` data structure is more appropriate here. Reversing the region now happens in seperate function.
These are no longer necessary.
… kwargs get passed to functions
This is so region_iter (the mutating arg) appears first in the function sig
|
Looks like there is an issue with |
|
I think that's fair enough. The sketch I had was that you would choose to either the method on the type if you didn't need any runtime info to generate the defaults, or the values in the event that you did. Ideally you wouldn't have both methods. I don't think that use case is relevant to anywhere in the code yet so I agree; a bridge we can cross when it becomes necessary. I can change to |
A callback occuring at each region can be passed using the `do` syntax using this method.
…into a function call.
|
Might as well include the |
|
This PR has a green light from me, after remaining review comments are resolved or answered. |
…..`. No kwargs get passed to these callbacks anyway so this is cosmetic change.
|
@jack-dunham Miles recommended just skipping the broken TTN fitting tests, it requires some more work to get those working. |
…ncluding `runtests.jl`.
|
The tests in the following files are broken in the branch of Considering this is a lot of tests, it might be worth investigating if most can be salvaged and ported into the new interface. I think the code that these tests were based on is in Perhaps it's best to merge this with those tests skipped and then deal with in the other PR. This PR doesn't break any further tests. i.e. with those tests skipped, all remaining tests pass. TODO:
|
… the region plan. This is to keep it consistant with other examples of `AbstractNetworkIterator`.
|
I think it makes sense to skip broken tests if they were already broken in Miles's PR. However, I would prefer that we left the file names as they were, and instead wrap the tests in @test_skip begin
@testset ...
end
endso it is recorded in the test results. Also, I'm a bit confused that all of those tests were broken, @emstoudenmire I thought you were using TTN DMRG and TDVP? |
Revert "Skip broken tests correctly" This reverts commit c675c6f64c9a13b46dcb36d292af12482ffff5c6. Delete depreciated code To be reintroduced in ITensorNetworksNext Squash of ^ This removes `solvers/previous_interfaces` and the broken test files
… defaults for the function it calls
To be reintroduced in a later date.
This PR attempts to define an interface for the iteration pattern that is similar to that of DifferentialEquations (as described in the Google doc), however in this case it was not as simple. I have described why and what I did below:
AbstractNetworkIteratorThe
iteratefunction in Julia really describes how one should transition between states, i.e. from A to B.For example, when iterating through
SweepIterator, one must define the newRegionIteratorfor the next sweep. This is something that is suitable to be wrapped initerateas it essentially is the step required to transition from one sweep to the next. What is less obvious is when to perform the region iteration itself such that the codeperforms the actual calculation. The region iteration is not really something that is part of "transitioning from sweep A to B" but rather work that is performed during a given sweep. This makes it somewhat difficult to reconcile the state of the data (such as the tensors themselves etc) with the state of the iterator.
Considering what iteration lowers to:
One has computed and iterated before any callback function. Each callback essentially acts before each computation, which is perhaps little awkward, but not necessarily a problem. What is a problem is that the first step is an exception to this; we don't get to execute a callback at step 1 before any computation happens.
To avoid this problem, and allow the callback to execute after the computation I have decide to separate out the "transition" step and the computation step into two functions
increment!andcompute!respectively. This also makes it clear which code is responsible for moving the iterator from A to B, and which code is responsible for performing computation while in state A (or B etc). We now also make the first call toincrement!implicit. That is, the iterator should, when initialized, run the necessary code such that it is ready for computation at the first step. In a way, this is as if we have transition from some abstract 0th state to the 1st state. In the first call to iterate only the call toincrement!is then skipped.This is the specific interface of
AbstractNetworkIterator. AnAbstractNetworkIteratoris a stateful iterator and can be summarized by the followingiteratedefinition:The function
donejust checks that the iteration is complete. Note, we increment before we compute, therefore we requirewith a
>=rather than>to avoid over shooting by 1. Simple subtypes ofAbstractNetworkIteratorcan use the above method by definingstateandBase.length, however one can instead choose to dispatch ondoneitself for more complicated cases (seeSweepIteratorfor an example).The code
when
iter::AbstractNetworkIteratornow executes the callback function after the computation step, but before the work has been done to transition to the next state.More details
compute!function be default just returns the iterator itself (and does nothing else). One can choose to return something else to be used in the body of a loop if desired.increment!has no default implementation on purposePauseAfterIncrementadapter iterator as one example of how the interface can be used. This is a general version of the proposed adapter that would allow manual iteration over the region.Other changes
Misc
SweepIteratornow has a single type parameterProblemthat can be used for dispatch (useful for default callbacks, see below).RegionIteratornow has an additional field,sweepthat is constant and simply stores the current sweep the region iteration is part of. This is to avoid constantly passing around thissweepkeyword when dispensing the sweep iterator in favor of the simpler region iterator.sweep_solvesweep_solveis now just a wrapper over the iterators that allows callbacks to be passed (and therefore requiresPauseAfterIncrement).sweep_iteratoras the only positional argument.kwargsfor now.sweep_callbackandregion_callbackfunctions have hasdefault_prepended to their function names to distinguish them from the keyword argument (and to make their purpose clear).Notes