Add an option to log progress while saving animations

[SBCV]

PR Summary

Closes #13561

Adds a parameter log_progress to anim.save(output_path, writer, log_progress=0)

Parameter description:
log_progress : int, optional
Controls the frequency of progress information written
during execution to the 'info' log. With log_progress=n
the progress information is written every time n frames
are processed. log_progress=0 disables this feature.

PR Checklist

• Has Pytest style unit tests
• Code is Flake 8 compliant
• New features are documented, with examples if plot related
• Documentation is sphinx and numpydoc compliant
• Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
• Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

[anntzer]

I would not bother adding a new parameter for that, but just always log the progress at INFO level.

[timhoffm]

Wouldn't it be reasonable to offer a generic callback, so that e.g. GUIs could do their own progress display?

[SBCV]

@anntzer the log_progress parameter does not influence the logging level, but allows the user to adjust the frequency of the logging information. If you want to animate thousands of frames, I think it does not make sense to print some information for each frame.

[SBCV]

@timhoffm I don't know. The current PR is simple and straight forward. But maybe there a good reasons to use a callback instead. Since I'm not a main developer of this library, I think someone else should decide this.

[anntzer]

But info-level logs are not displayed by default; you need to set the log level explicitly to see them. So it's no big deal to emit a bunch of logs at that level (well, preferably not at import time or when doing really common operations, but I think logging every frame on animation saves is just fine).

[SBCV]

Side note: In my case, I load many numpy arrays from disc and plot each of them with the animation tool. When printing a log for each frame, it will be difficult to keep track, which animation is currently processed and what the overall progress of the script is. That is why I provided this parameter.
But I understand that you want to keep the interface as clean as possible. I just wanted to point out this limitation.
What about the callback proposed by timhoffm? It would be nice if we could agree to a common position.
I'll update the PR then.

[timhoffm]

My argument for a callback: In many cases (and the default) a progress indicator is not necessary. But when the progress should be indicated, use cases can be quite diverse, which cannot be met with a hard-coded logging call. Instead we should offer a simple way to flexibly hook in the desired functionality. That works via a callback, e.g.

animation.save(filename,
               progress_callback=lambda i, n: _log.info(f'Saving frame {i} of {n}'))

or

def update_progress(i, n):
    if (i % 10) == 0:
        _log.debug(f'Saving frame {i} of {n} to {filename}')
animation.save(filename, progress_callback=update_progress)

or

progressbar = QProgressBar()
progressbar.maximum = nframes

def update_progressbar(i, n):
    progressbar.value = i

animation.save(filename, progress_callback=update_progressbar)

[anntzer]

A callback is fine for me.

[timhoffm]

The only question: Should it have one argument giving the current frame or should there be a second argument giving the total number of frames? While strictly speaking, the first is sufficient (you can get the total number in a different way), it may be handy to have the total number passed as well.

[jklymak]

Its a bit of a steep climb for the naive user to write a callback; if we are adding a kwarg anyways, perhaps its OK to let this PR is as-is, and let a future PR allow log_progress accept a callback as well as an integer?

[timhoffm]

I'm against providing a simple logging functionality. Partly, because it's just one special case, and I don't see that it's more suitable than other mechanisms. Partly, because we're getting into naming issues for the parameter. log_progress would be quite a bad name for a callback parameter.

Adding a callback is not more difficult than list.sort(key=lambda x: x.value), so the pattern of passing functions as parameters should be well known.

Conversely, logging needs understanding/adaption of the log levels. This is not a naive user topic. Just emitting infos will not work.

Furthermore, I argue that getting progress feedback for animation saving is not a required feature for the naive user. We don't have to bend the API for them on this feature. It's off by default and the user has to consult the docs anyway. We can add a simple example such as progress_callback=lambda i, n: print(f'Saving frame {i} of {n}') to the docstring, which should give also inexperienced users the ability to use that feature.

[SBCV]

Replaced the parameter with a callback. Is the PR in a good state now? Anything else I should adjust?

[timhoffm]

Essentially, this is good. Only some minor stylistic issues.

[SBCV]

Adjusted the style according to your comments

[timhoffm]

Please work out the formatting issues to get CI pass.

• Doc build fails due to a ReST formatting issue.
• flake8 (Travis run) complains about too long lines.

[tacaswell]

👍 This is a nice new feature! It looks like you could also inject tqdm via this callback.

This will definitely need an entry in whats_new

[SBCV]

What do you exactly mean with whats_new. Is this something I can do? Or has this to be done by one of the maintainers?

[timhoffm]

For a what's new entry add a file matplotlib/doc/users/next_whats_new/[date]-[initials]-[optional-info].rst. There are already a number of these files in the folder which you can use as a template.

[timhoffm]

Apart from total_frames=None and the what's new entry, this is ready to go.

[SBCV]

I squashed the changes, is there anything else to do?

[timhoffm]

I don't think there's anything more you need to do. We just need a second review from one of the core devs.

[SBCV]

perfect, thank you for your support

[anntzer]

Anyone can merge after progress_callback is made keyword only (or if a core dev thinks it's not worth it).

[timhoffm]

Doc failure is is only due to being based on the recent problems with warnings in doc build. Would be fixed by rebasing, but saving the hassle here.

[tacaswell]

Would have preferred to not have had this backported, but definitely not worth reverting the backport.