odlgroup
Callback: step-parameter accepts list of steps
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
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
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__')
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
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?
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
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?
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
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.
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 stepsis 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.
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 ;)
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 aboutits = 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 :grimacing:, 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, :exploding_head:). 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, ...