Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Caching figures generated by plot directive#25091
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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 grammerExamples of using rst need to be code blocksUse logging instead of printSatisfy PEP8 checksFIX: issue with rebase due to re-naming of local variablesSTY: fix line length and indentation issuesSTY: trim whitespace and add trailing commaFix doc styleflake8 fixesplot_preserve_dir > plot_cache_dirCopy edit changelog entry
c78c810 tofca5eb3CompareHow 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am slightly skeptical of the first claim (that users can give things good names) andvery 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We do not actually useoutname for anything other than checking it exists!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 useoutname 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? |
tacaswell commentedJan 27, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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