Callback: step-parameter accepts list of steps

[miallo]

PR to #1541
It most probably isn't PEP8 compliant (there is no char-count in the Web-Editor...) and the naming can be debated as well

[pep8speaks]

Checking updated PR...

• In the file odl/solvers/util/callback.py:

Line 1048:1: E302 expected 2 blank lines, found 1

Comment last updated at 2020-03-06 16:45:15 UTC

[miallo]

The casting of the items should probably be changed to self.steps = [int(step) for step in steps] and the check in the init should be changed to if hasattr(step, '__iter__')

[adler-j]

Looks good overall. I think the correct way in python is to try-catch iter(step) though, see e.g. https://stackoverflow.com/questions/1952464/in-python-how-do-i-determine-if-an-object-is-iterable

[miallo]

If one inputs e.g. step=[[1],2] one still get's a TypeError, but from trying to cast to a single int([[1],2]) because of the try/except. But I guess that isn't that important?

[miallo]

Do you think the code should be refactored into a method that is called in all relevant places?

def _setupShouldEvaluateAtStep(self, step):
    try:
        self.step = frozenset(int(i) for i in step)
        self.should_evaluate_at_step = lambda i: i in self.step
    except TypeError:
        self.step = int(step)
        self.should_evaluate_at_step = lambda i: i % self.step == 0

I think 1) it makes the inits more readable and 2) it makes it easier to keep track of changes in a single place

[miallo]

One could even think of putting the whole step and iter handling/incrementing into a decorator, so that the code is easier to maintain (#1538). It is not possible for the __init__, since the positional arguments are in different places, but the __call__ could very easily be decorated to prevent such bugs.

On that exact note: do I get it wrong, or does the CallbackShow.__call__ handle it incorrectly? If the space_of_last_x changes for step that is skipped then the update in the next step might be "in place", even if it changed since the last plot?

[miallo]

Something like this maybe?

from functools import wraps

def call_if_should_be_evaluated(func):

    @wraps(func)
    def wrapper(self, *args, **kwargs):
        if self.should_evaluate_at_step(self.iter):
            func(self, *args, **kwargs)
        self.iter += 1

    return wrapper

[kohr-h]

I like the general idea to support varying step size in callbacks. The current version, however, combines two different ways of determining when to evaluate the callback: either after a number of steps or at given steps. IMO they should either be two separate mutually exclusive arguments (not great), or the list of steps should also mean the steps between consecutive evaluations. In other words, step=1 whould then be equivalent to step=[1, 1, ...]. The iterations at which to evaluate would be given by np.cumsum([0] + list(steps)), and conversely, steps can be calculated from iterations using np.diff.

That would raise the question what should happen when the list of steps is exhausted. One option would be to keep going with the last step value in the list (then step=1 is the same as step=[1]). Alternatively, one could stop after the last step has been used. What's your preference? Also ping @adler-j @aringh

Implementation-wise I think the best solution would be to use a generator that is stored in the instance (instead of storing a function, the step and the current iteration separately). For instance:

class Callback(object):
    def __init__(self, ..., step=1):
        try:
            int(step)
        except TypeError:
            step_iter = iter(step)
        else:
            def step_iter():
                while True:
                    yield step

        def should_evaluate():
            yield True  # always run at first iteration
            while True:
                cur_step = next(step_iter)
                for _ in range(cur_step - 1):
                    yield False
                yield True

        self.should_evaluate = should_evaluate

    # This is implemented by subclasses
    def __call__(self):
        should_eval = next(self.should_evaluate)
        if should_eval:
            # run logic

This solution would also immediately support iterators instead of finite collections.

[kohr-h]

Why use a different function name style?

[kohr-h]

Attributes should only be set in __init__ if possible, not in "free" functions like this.

[miallo]

I wanted to refactor the whole repetition and thought about returning self, so that the user does see that self is modified inside of this function. Otherwise there is a lot of code repetition and the inits are quite bloated if the user wants to take a look at them.
I also thought about a metaclass that will automatically setup the the init and call, but I guess that is a bit of an overkill

[miallo]

always run at first iteration

Is there a reason why this must be done?
The rational behind my interpretations of a list of steps is that the user does have full control at which steps he wants to evaluate the functional. E.g. I want to save images/movie frames but only once it already started converging.

That would raise the question what should happen when the list of steps is exhausted.

Another consideration could be itertools.cycle, so that they loop indefinitely.

I do understand that there is a big difference in the interpretation of integers and lists in my implementation.

In which cases / with which Callbacks would the user opt for variable step sizes instead of fixed? I would argue that they want to perform tasks at specific steps. If I want to save images at step 5, 50 and 100 I would need to write step=[5, 45, 50]. Is this intuitive? And now I "accidentally" saved the fist iteration as well.

Isn't the question "What happens after exhaustion" without an obvious answer a hint that the notion of steps between iterations is flawed in this context? I for myself think a list of steps at iterations is the more logical interpretation, even if there is a difference in the interpretation of at/between for Ints and lists.

[kohr-h]

always run at first iteration

Is there a reason why this must be done?

Only reason is that this happens for integer step. It's not a must, though. We still have freedom in how we interpret the list. For instance, we could take the first number as the step from 0, so if you want evaluation at the first iteration, the first entry in the list should be 0.

The rational behind my interpretations of a list of steps is that the user does have full control at which steps he wants to evaluate the functional. E.g. I want to save images/movie frames but only once it already started converging.

Yes, I fully understand and agree. We should support "starting late".

Another consideration could be itertools.cycle, so that they loop indefinitely.

Right. In that case I tend towards the default to stop when the list is exhausted. The other cases can then be supported by wrapping the list in a generator that cycles, repeats, or does anything you like.

In which cases / with which Callbacks would the user opt for variable step sizes instead of fixed? I would argue that they want to perform tasks at specific steps. If I want to save images at step 5, 50 and 100 I would need to write step=[5, 45, 50]. Is this intuitive? And now I "accidentally" saved the fist iteration as well.

This is not very intuitive or easy to handle, agreed. To fix it, we could offer an alternative constructor that takes a list of evaluation points instead of steps, e.g.,

class Callback(object):
    def __init__(self, ...):
        # as usual

    @classmethod
    def from_iterations(cls, its, *args, **kwargs):
        its = iter(its)

        def step_iter():
            next_it = next(its)
            while True:
                try:
                    cur_it, next_it = next_it, next(its)
                except StopIteration:
                    return
                step = next_it - cur_it
                assert step > 0
                yield step

        return cls(*args, step=step_iter, **kwargs)

And construction then works as Callback.from_iterations([1, 10, 100, 1000], ...). What's nice about his is that subclasses don't need to be changed (I believe).

Side note: If I were to start again from scratch, I'd implement the callbacks with plain Python generators and scrap the classes. Alternative constructors would then just be conversion functions.

Isn't the question "What happens after exhaustion" without an obvious answer a hint that the notion of steps between iterations is flawed in this context?

I'd say no, because the callback cannot know when it will stop and thus has to assume that it will run indefinitely. It has to somehow deal with an unbounded number of iterations, and just stopping after the last iteration in its list is only one possible solution. I'd argue, on the contrary, that extending a list of steps to infinity is much more obvious than extending a list of evaluation points.

[kohr-h]

Any new thoughts on this stuff?

[miallo]

Sorry for not replying, but I needed to help my grandmother for the last few weeks...

Your proposal needs assert step > 0. What about its = iter(sorted(list(set(its))))? This will sort the list and remove duplicates. No need for assertions and easier for the users, since they can pass in an unordered list and accidentally one step multiple times. Negative inputs could screw this up, but I guess we can trust the users this much (or one could ignore them).

BTW: I totally agree with you about stoping after exhaustion. It was just an additional example on what strange things could be thought of ;)

[kohr-h]

Sorry for not replying, but I needed to help my grandmother for the last few weeks...

Don't worry.

Your proposal needs assert step > 0. What about its = iter(sorted(list(set(its))))? This will sort the list and remove duplicates. No need for assertions and easier for the users, since they can pass in an unordered list and accidentally one step multiple times.

Sounds like a good suggestion. We tend to be liberal about input values. Maybe it's good to clarify then that duplicates will not lead to the callback running multiple times.

Negative inputs could screw this up, but I guess we can trust the users this much (or one could ignore them).

Hm, makes me realize that my suggested implementation has a bug 😬, namely that the first step in the generator should be from 0 to its[0], not from its[0] to its[1]. With sorting added, we have the opportunity to check that the first step is nonnegative and raise an error otherwise.

---

To improve usability, I'd suggest pulling out the "iterations-to-steps" conversion into a public function (a function returning a generator, 🤯). That gives the opportunity to "extend" that generator in interesting ways, e.g., wrap it in a new generator that yields all values and then the last one indefinitely:

def repeat_last(gen):
    for val in gen:
        yield val
    while True:
        yield val

Or other stuff from itertools like cycle, chain, ...