Changes to the iterator interface

[jack-dunham]

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:

AbstractNetworkIterator

The iterate function 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 new RegionIterator for the next sweep. This is something that is suitable to be wrapped in iterate as 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 code

for _ in sweep_iterator end

performs 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:

next = iterate(iter)
while next !== nothing
    (item, state) = next
    # body (this is where any call to a callback function would occur)
    next = iterate(iter, state)
end

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! and compute! 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 to increment! 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 to increment! is then skipped.

This is the specific interface of AbstractNetworkIterator. An AbstractNetworkIterator is a stateful iterator and can be summarized by the following iterate definition:

function Base.iterate(NI::AbstractNetworkIterator, init=true)
  done(NI) && return nothing
  init || increment!(NI)
  rv = compute!(NI)
  return rv, false
end

The function done just checks that the iteration is complete. Note, we increment before we compute, therefore we require

done(NI::AbstractNetworkIterator) = state(NI) >= length(NI)

with a >= rather than > to avoid over shooting by 1. Simple subtypes of AbstractNetworkIterator can use the above method by defining state and Base.length, however one can instead choose to dispatch on done itself for more complicated cases (see SweepIterator for an example).

The code

for _ in iter
    callback(iter)
end

when iter::AbstractNetworkIterator now executes the callback function after the computation step, but before the work has been done to transition to the next state.

More details

• The 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.
• The function increment! has no default implementation on purpose
• See the PauseAfterIncrement adapter 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

• SweepIterator now has a single type parameter Problem that can be used for dispatch (useful for default callbacks, see below).
• I've changed some function calls to be explicit calls to constructors to make it clearer when one is constructing rather than getting.
• RegionIterator now has an additional field, sweep that is constant and simply stores the current sweep the region iteration is part of. This is to avoid constantly passing around this sweep keyword when dispensing the sweep iterator in favor of the simpler region iterator.

sweep_solve

• The function sweep_solve is now just a wrapper over the iterators that allows callbacks to be passed (and therefore requires PauseAfterIncrement).
• The callback functions now take the entire sweep_iterator as the only positional argument.
• I've allowed passing in arbitrary kwargs for now.
• The sweep_callback and region_callback functions have has default_ prepended to their function names to distinguish them from the keyword argument (and to make their purpose clear).

Notes

• I tested the relevant tests in solvers subdirectories but I couldn't get the full test suite to run for some reason. Might be something up with my environment.
• I will add some unit tests!

[jack-dunham]

• Reconsider the done function. The name is misleading as the computation is not "done". The purpose of this function is really to check if the iterator is in the final state.
• Rename PauseAfterIncrement to something more sensible, i.e. NoCompute or similar.
• Add an adapter that spits out the region iterator EachRegion, e.g.
• Consider if it makes sense to embrace mutability during the region_step function. Perhaps this then informs the interface for extract insert and update.
• Consider rolling which_sweep field of SweepIterator into RegionIterator.

[mtfishman]

Suggested change

	sweep_kws = Iterators.Stateful(sweep_kws)
	stateful_sweep_kws = Iterators.Stateful(sweep_kws)

I would find that a bit clearer (i.e. it is easier to keep track that we are now using the stateful version of sweep_kws).

[mtfishman]

Should we parametrize SweepIterator by the type of sweep_kws? Or are we expecting it could be modified such that it changes type (though if that is the case we could make it explicit by setting the type parameter to Any)?

[jack-dunham]

Just wondering what would be the advantage of having this parameterized? I think the power of this SweepIterator object comes from that fact that it can be basically any object implementing the iterate interface (for example, an iterator that loads the kwargs from a file, even). The eltype of the iterator should be a NamedTuple, but then we cannot define this type necessarily as different kwargs then lead to different concrete NamedTuple types.

[mtfishman]

Purely for the sake of type stability, in cases where that is possible. I was just making the point that by parametrizing and then setting the default to Any we could allow for either option but allow it to be dynamic by default, but we could add that option later if needed.

Also I would recommend ordering the fields in the struct as region_iter, sweep_kws, which_sweep, that seems more natural to me since we are inputting the problem argument as the first argument and generally dispatching on the problem.

[mtfishman]

Passing outputlevel is a bit funny to me (I think it is there for historical reasons). I would think that it would be easy enough to pass keyword arguments as part of the callback, so I think it can be removed.

[mtfishman]

Otherwise, we could have users pass sweep_callback_kwargs and region_callback_kwargs NamedTuples.

So basically the alternatives would be:

sweep_solve(sweep_iterator; region_callback=(x -> default_region_callback(x; outputlevel=1))

or:

sweep_solve(sweep_iterator; region_callback_kwargs=(; outputlevel=1))

I'm ok with either one, though I'd bias towards the first one for now since we can always add the second one later, for the sake of keeping this level of the code simpler.

[jack-dunham]

I don't agree with passing outputlevel as a kwarg, I agree its a bit weird, and I left it in for historical reasons as you mention.

IMO, options like outputlevel can be defined as part of a closure in a wrapper function and then exposed to a user, if we (or another user) wishes to do that. We should not assume a notion of outputlevel for all callbacks.

[mtfishman]

Suggested change

	return sweep_solve(sweep_iterator; sweep_callback=sweep_callback, kwargs...)
	return sweep_solve(sweep_iterator; sweep_callback, kwargs...)

[mtfishman]

Maybe it's better to remove the kwargs... to make sure we don't accidentally pass unsupported keyword arguments.

[jack-dunham]

This was again an artifact of supporting outputlevel for the already written default callbacks. I agree it should not be supported.

[mtfishman]

Maybe it's better to use field access here, i.e. sweep_iterator.problem.

[jack-dunham]

Ah SweepIterator has no field problem. RegionIterator is the object with the field problem. Using a field getter means we can obtain the Problem object the same way for both SweepIterator and RegionIterator.

[mtfishman]

Just a small recommendation, I would prefer a variable name besides NC, maybe we could just use iterator.

[mtfishman]

Related to our discussion around how the RegionIterator gets updated and how the problem gets passed around, maybe it could help to wrap this constructor call into a function, for example:

function update_region_iterator(iterator::RegionIterator; kwargs...)
  return RegionIterator(iterator.problem; kwargs...)
end

or something like that (mostly to hide the detail of how the problem gets passed along at this level of the code).

[mtfishman]

Also, what does SR stand for? Generally I prefer lower case variable names, and also more descriptive ones, say sweep_iter.

[jack-dunham]

I think SR was an abbreviation of SweepRegionIterator which was a previous name for something I was using previously...

On the first point, I feel like update_region_iterator is not really descriptive of what the function as implemented there does. It does not update an existing RegionIterator, it creates a new one via the constructor. I do like however the following:

function update_region_iterator!(iterator::SweepIterator; kwargs...)
    iterator.region_iter = new_region_iterator(iterator.region_iter; kwargs...)
    return iterator
end

where new_region_iterator is a renaming of the method you describe. Let me know if you agree.

[mtfishman]

It does not update an existing RegionIterator, it creates a new one via the constructor. I do like however the following:

I suppose that's a matter of perspective, i.e. we could think of it as a fancy kind of "setter" method that keeps some properties and sets other ones, which for immutable types has to create a new object by definition, but I'm ok with new_region_iterator.

[jack-dunham]

Some comments:

Bearing in mind the discussions about avoiding the proliferation of methods that don't do a lot (i.e. just pass data from structs as arguments), I have elected to take the opposite approach regarding the interface defined for default_kwargs.
This is, however, for good reason; most of the defaults don't actually use the contents problem (or region_iter for that matter), i.e. it is just used for dispatch, therefore it makes sense to have users dispatch on the type itself as one can then access these defaults without constructing an instance of said type.

If one needs some of the runtime data from problem or whatever, they can still just define a method dispatching on AbstractProblem like normal as now it does not even make sense to have this default accessible without an instance of this type as the default depends on values at runtime...

For that reason, I think supporting both makes sense. One can define a method in two ways:

default_kwargs(f, ::MyProblem)) # value domain
default_kwargs(f, ::Type{<:MyProblem)) # type domain

If no method in the value domain is set, then it falls back to the one in the type domain. I have (briefly) described the interface in the docstring of default_kwargs.

Some more things to discuss:

1. Which functions should support this interface
2. If we should support defaults that are not tied to a function
3. Avoiding type piracy

On the last point, we should be strict about asking users to only define default_kwargs methods on their own types and functions. It would therefore be sensible to have default methods defined by us to have signatures default_kwargs(f, ::AbstractProblem) as then one cannot redefine this method without getting a warning (I think/hope??). We then need a system for overriding these defaults which can be done with a ScopedValue.

[mtfishman]

That sounds reasonable to me.

[jack-dunham]

Fix:

Suggested change

	increment!(adapter::EachRegion) = state(adapter.parent)
	increment!(adapter::EachRegion) = increment!(adapter.parent)

But this should really flatten nested iterator into an iterator over all regions.

[jack-dunham]

Marry this with function signatures.

[jack-dunham]

Have this splat kwargs into default_kwargs.

[jack-dunham]

This can be removed.

[jack-dunham]

Suggested change

	struct NoComputeStep{S<:AbstractNetworkIterator} <: AbstractNetworkIterator
	struct JustIncrement{S<:AbstractNetworkIterator} <: AbstractNetworkIterator

[jack-dunham]

The default_kwargs system has been overhauled somewhat in light of discussions between @mtfishman and I. It is now no longer as tailored to the RegionIterator context, and should instead by used to make keyword arguments of any function method shareable.

Because of this, getting and splatting kwargs from the RegionIterator no longer happens in one function call. In the code one should obtain the kwargs for a function func from the RegionIterator named e.g. iter using

func_kwargs = region_kwargs(func, iter)

and then do something like

func(arg1, iter, arg2; default_kwargs(func, arg1, iter, arg2; func_kwargs)...)

to do the default overwriting. The reason for this is that default_kwargs(::typeof(func), args...) should mirror the signature of func. This means that there is no consistency between where RegionIterator enters a given function, if it does at all.

This system is now more or less independent of the solvers code, but perhaps it is best to wait for the rewrite before considering if it's useful across the board of not.

I just need the fix the broken tests in test_default_kwargs.jl (they assumed the previous implementation), but I think that's everything for now. I also think the following macro would be useful:

@with_defaults func(arg1, iter, arg2; func_kwargs...)
# expands to
func(arg1, iter, arg2; default_kwargs(func, arg1, iter, arg2; func_kwargs)...)