Caching figures generated by plot directive #25091
Conversation
|
as a note, you should be able to force-push to that branch @dstansby . |
Fix bug with adding null string to outname_list
Futzing with PEP8 whitespace stuff. Fix grammer Examples of using rst need to be code blocks Use logging instead of print Satisfy PEP8 checks FIX: issue with rebase due to re-naming of local variables STY: fix line length and indentation issues STY: trim whitespace and add trailing comma Fix doc style flake8 fixes plot_preserve_dir > plot_cache_dir Copy edit changelog entry
c78c810 to
fca5eb3
Compare
|
How does this interact with the changes to the plot directive introduced in #24205? My initial reaction is that the |
| e.g. :file:`docfile3-4-01.png` or something equally mysterious. With | ||
| ``:outname:`` the figure generated will instead be named | ||
| :file:`stinkbug_plot-01.png` or even :file:`stinkbug_plot.png`. This makes it | ||
| easy to understand which output image is which and, more importantly, uniquely |
There was a problem hiding this comment.
I am slightly skeptical of the first claim (that users can give things good names) and very skeptical of the second as the name is not random, it is based on the name of the file (and the position + function of the directive in that file) so is structurally unique. If users can control the names there is a high chance that they are going to collide.
If we want to make things save to cache like this we should give them names based on the hash of the source....
| @@ -613,6 +635,12 @@ def render_figures(code, code_path, output_dir, output_base, context, | |||
| for fmt, dpi in formats: | |||
| try: | |||
| figman.canvas.figure.savefig(img.filename(fmt), dpi=dpi) | |||
| if config.plot_cache_dir and outname is not None: | |||
There was a problem hiding this comment.
We do not actually use outname for anything other than checking it exists!
There was a problem hiding this comment.
I have some conceptual concerns:
- name are already (structurally) unique
- do not trust users to give things unique names (although it does check that)
- the cache is not invalidated if the code in the directive changes
and an implementation concern (we do not seem to actually use outname in the code!)
I think this is a good idea, but probably should cache the files based on a hash of the source rather than a user supplied name.
|
Sorry for doing a 'blind' rebase and not really thinking this through myself, but thanks for re-reviewing! Just thinking about the key for the cache, hasing the source code is potentially no though good because changes in Matplotlib internals (e.g. a change in default style, a bugfix to a plotting method) would lead to a different image with the same source code. So I think we either have to:
I'm torn between these two options, but am erring towards number 1. due to the potential of number 2. to cause comfusion to users at some point down the line. Thoughts? |
I went back to the original PR where I also did a blind rebase and am to blame for at least some of these issues! For the key we could start the hash with So given the caching problem (nb: there are two hard problems in computer science: naming things, cache invalidation, and off-by-one bugs) I'm inclined to go with option 1 as well. |
|
In that case I'll close. @r-barnes thanks a lot for opening this PR originally - we decided not to carry it forward because of the difficulties of caching the images. See the comment above this one for more context. I think we would be open to re-visiting this, but that would require a well thought out strategy for caching the images in an issue before opening a pull request. |
…n getter setter for subplot param and figsize
PR Summary
A rebase of #10149 onto current main, with conflicts solved, and the latest feedback on that PR taken into account. For context see the description of #10149.
PR Checklist
Documentation and Tests
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst