Repository navigation
Caching figures generated by plot directive - #25091
dstansby wants to merge 4 commits into
Conversation
Sorry, something went wrong.
|
as a note, you should be able to force-push to that branch @dstansby . |
Sorry, something went wrong.
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
|
How does this interact with the changes to the plot directive introduced in #24205? My initial reaction is that the outname change should be orthogonal, but the plot_cache_dir may conflict and/or be totally ignored. |
Sorry, something went wrong.
| 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....
Sorry, something went wrong.
| 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!
Sorry, something went wrong.
tacaswell
left a comment
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, something went wrong.
|
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? |
Sorry, something went wrong.
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 matplotlib.__version__, but for developers that would quickly lead to an ~unbounded cache as we put the git sha in the version and would only move the problem out a level to any plot directive that uses Matplotlib via a library (e.g. seaborn) having exactly the same problem but not participating in the hash (and it seems this would be general to any library used in the code...). 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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
…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
Release Notes