Added CallbackMovie

[miallo]

Code for #1135

[pep8speaks]

Checking updated PR...

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

Line 1081:80: E501 line too long (88 > 79 characters)
Line 1048:1: W293 blank line contains whitespace

Comment last updated at 2020-03-30 19:01:09 UTC

[miallo]

What is the minimum supported Python2 version? If it is 2.5 or below we need from __future__ import with_statement

[kohr-h]

What is the minimum supported Python2 version? If it is 2.5 or below we need from __future__ import with_statement

No, the minimum is 2.7. See here:

odl/setup.cfg

Lines 30 to 35 in 0d06b07

	Programming Language :: Python :: 2
	Programming Language :: Python :: 2.7
	Programming Language :: Python :: 3
	Programming Language :: Python :: 3.5
	Programming Language :: Python :: 3.6
	Programming Language :: Python :: 3.7

and here:

odl/setup.cfg

Line 46 in 0d06b07

python_requires = >=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*, !=3.4.*

[kohr-h]

Thanks for your contribution @miallo. This callback can certainly be useful in many situations.

But in the current implementation as a single class, I think that too much unrelated functionality is tied up in one place. The class has trouble managing its own state correctly and must use some weird logic to avoid wrong usage.

My suggestion would be to pull the context manager out of the class and make it a free context manager that provides a callback when entered. Like so:

@contextlib.contextmanager
def save_animation(writer):
    # Init code
    writer = ...
    im = ...

    class CallbackAppendMovieFrame(Callback):
        def __call__(self, x):
            # Do something with x
            im.set_array(...)
            writer.grab_frame()

        def __repr__(self):
            # Not so important anymore since the class cannot
            # be instantiated directly anyway

    try:
        with writer.saving(...):
            yield CallbackAppendMovieFrame()
    finally:
        # Whatever needs to be finalized, closed, ...

This way, the callback can only be used in the intended way.

Whether the figure should be created outside or in the context manager I don't really know yet. I guess most of the time one would use this with the agg backend anyway that doesn't show anything on screen. Is there a specific reason you put it outside in the current implementation, @miallo?

[miallo]

My suggestion would be to pull the context manager out of the class and make it a free context manager that provides a callback when entered.

One thing to consider is: if users type odl.solvers.Callback and look at the autocomplete, they will not see this. I know it isn't the best reason, but this is how I discovered the ones I use.

Whether the figure should be created outside or in the context manager I don't really know yet. I guess most of the time one would use this with the agg backend anyway that doesn't show anything on screen. Is there a specific reason you put it outside in the current implementation, @miallo?

I did this to easily set up the colormap, and x/y-limits inside the plt.imshow but I agree that it isn't nice. And you can do it with rcParams["image.cmap"] = 'viridis', so creating it is a good idea. It does make the handling easier and less error prone.

[kohr-h]

My suggestion would be to pull the context manager out of the class and make it a free context manager that provides a callback when entered.

One thing to consider is: if users type odl.solvers.Callback and look at the autocomplete, they will not see this. I know it isn't the best reason, but this is how I discovered the ones I use.

But also not the worst :-)
In hindsight, I guess we over-optimized a bit for discoverability by naming everything Callback.... When names are a bit less uniform, people will look around more, perhaps even in the docs 👍

That said, I'd still advocate the standalone context manager.

Whether the figure should be created outside or in the context manager I don't really know yet. I guess most of the time one would use this with the agg backend anyway that doesn't show anything on screen. Is there a specific reason you put it outside in the current implementation, @miallo?

I did this to easily set up the colormap, and x/y-limits inside the plt.imshow but I agree that it isn't nice. And you can do it with rcParams["image.cmap"] = 'viridis', so creating it is a good idea. It does make the handling easier and less error prone.

Ah I see. Indeed, global settings can be modified outside using matplotlib rcParams (or the .rc() function). But maybe it's really useful to have a handle outside to the figure and thereby also to its contents. If you agree, make it optional so users aren't forced to instantiate it but can if they want.

[miallo]

The current implementation of the whole callback only works for (2D-)images - if it was used for 1D plots as well, there are a couple of things that had to be changed...

Is there a specific reason you put [the figure] outside in the current implementation, @miallo?

Now I remember the main reason why I did this: I cannot create a plt.imshow instance without an array of the shape of the images. The workaround is in my last commit, but it isn't nice...

[kohr-h]

Thanks for the update, looking very promising.

The current implementation of the whole callback only works for (2D-)images - if it was used for 1D plots as well, there are a couple of things that had to be changed...

I think it's fine for now to only support the most relevant case. To make that clearer, the callback could raise a NotImplementedError if x.ndim != 2 (or not hasattr(x, 'ndim').

Is there a specific reason you put [the figure] outside in the current implementation, @miallo?

Now I remember the main reason why I did this: I cannot create a plt.imshow instance without an array of the shape of the images. The workaround is in my last commit, but it isn't nice...

Ah, that makes sense. I don't think the workaround is that bad, though. To make it even nicer, you could move the im logic fully into the callback, i.e., get the ax object outside and in each call

try:
    im = ax.get_images()[-1]
except IndexError:
    im = ax.imshow(x)

That would remove the necessity to overwrite a nonlocal.

---

BTW, we don't have to create a Callback class internally. The solvers also accept a plain Python function, so if you like you can scrap the class and just write the __call__ part as a function that you yield instead of the class instance. Up to you.

[miallo]

BTW, we don't have to create a Callback class internally. The solvers also accept a plain Python function, so if you like you can scrap the class and just write the __call__ part as a function that you yield instead of the class instance. Up to you.

A Callback has the major advantage that you can easily combine it with an odl-operator by "multiplication" and that you can combine multiple callbacks with &...

[kohr-h]

BTW, we don't have to create a Callback class internally. The solvers also accept a plain Python function, so if you like you can scrap the class and just write the __call__ part as a function that you yield instead of the class instance. Up to you.

A Callback has the major advantage that you can easily combine it with an odl-operator by "multiplication" and that you can combine multiple callbacks with &...

Good point, let's stick to Callback then.

[miallo]

I extracted plot_in_figure into a separate method if one wants to plot non 2D data. Is this unnecessary?

What else needs to be done?

[kohr-h]

Some comments and suggestions mostly on documentation and style. Functionality-wise I think we're good, looks nice and clean.

(We're in the nit phase now 😉 ).

[kohr-h]

I pushed a few doc and default behavior improvements, after that I'll merge.