Skip to content

Plot directive preserve #10149

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from

Conversation

r-barnes
Copy link

@r-barnes r-barnes commented Jan 3, 2018

PR Summary

The Sphinx plot directive can be used to automagically generate figures for documentation like so:

.. plot::

   import matplotlib.pyplot as plt
   import matplotlib.image as mpimg
   import numpy as np
   img = mpimg.imread('_static/stinkbug.png')
   imgplot = plt.imshow(img)

But, if you reorder the figures in the documentation then all the figures may need to be rebuilt. This takes time. The names given to the figures are also fairly meaningless, making them more difficult to index by search engines or to find on a filesystem.

Alternatively, if you are compiling on a limited-resource service like ReadTheDocs, you may wish to build imagery locally to avoid hitting resource limits on the server. Using the new changes allows extensive dynamically generated imagery to be used on services like ReadTheDocs.

The :outname: property

These problems are addressed through two new features in the plot directive. The first is the introduction of the :outname: property. It is used like so:

.. plot::
   :outname: stinkbug_plot

   import matplotlib.pyplot as plt
   import matplotlib.image as mpimg
   import numpy as np
   img = mpimg.imread('_static/stinkbug.png')
   imgplot = plt.imshow(img)

Without :outname:, the figure generated above would normally be called, e.g. docfile3-4-01.png or something equally mysterious. With :outname: the figure generated will instead be named stinkbug_plot-01.png or even stinkbug_plot.png. This makes it easy to understand which output image is which and, more importantly, uniquely keys output images to code snippets.

The plot_preserve_dir configuration value

Setting the plot_preserve_dir configuration value to the name of a directory will cause all images with :outname: set to be copied to this directory upon generation.

If an image is already in plot_preserve_dir when documentation is being generated, this image is copied to the build directory thereby pre-empting generation and reducing computation time in low-resource environments.

Related issues were raised on ReadTheDocs (link) and on StackOverflow (link).

PR Checklist

  • Has Pytest style unit tests
  • It would, but I'm not sure how to go about writing them. Sorry :-/
  • Code is PEP 8 compliant
  • Leastwise, nothing new is added to the numerous pre-existing violations. PEP8 adherence makes code difficult to read :-/
  • 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

@tacaswell tacaswell added this to the v2.2 milestone Jan 3, 2018
@tacaswell
Copy link
Member

👍 in principle, but the docs need to build cleanly!

@@ -656,6 +674,9 @@ def render_figures(code, code_path, output_dir, output_base, context,
for format, dpi in formats:
try:
figman.canvas.figure.savefig(img.filename(format), dpi=dpi)
if config.plot_preserve_dir and outname:
print("Preserving '{0}' into '{1}'".format(img.filename(format), config.plot_preserve_dir))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not reviewing, but I don't think you want print in here do you? I'd do _log.info or _log.debug?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's probably what I want, but I didn't know about them. I'll update.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I don't see _log as a variable in the plot_directive.py namespace, so I'm not sure how to make that adjustment.

Copy link
Member

@jklymak jklymak Jan 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its not already in this module, then up at the top:

# ... other imports
import logging
# ... other imports

_log = logging.getLogger(__name__)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks: got that up.

@r-barnes
Copy link
Author

Eliminated some spaces

@jklymak jklymak modified the milestones: needs sorting, v3.0 Mar 16, 2018
@jklymak jklymak modified the milestones: v3.0, v3.1 Jul 9, 2018
@tacaswell tacaswell modified the milestones: v3.1, v3.2 Feb 1, 2019
@jklymak
Copy link
Member

jklymak commented Jul 24, 2019

@r-barnes sorry this didn't get a second review yet. If you give it a rebase and ping me I'll give it a quick look and merge on the basis of @tacaswell 's approval. We are about to branch 3.2, so it'd be nice to get this in

@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 24, 2019
@timhoffm
Copy link
Member

Wouldn't plot_preserve_dir better be named plot_cache_dir?

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly the indentation needs to be fixed.

@r-barnes I am going to do the rebase and force-push to your branch

if config.plot_preserve_dir and outname:
outfiles = glob.glob(os.path.join(config.plot_preserve_dir, outname) + '*')
for of in outfiles:
_log.info("Copying preserved copy of '{0}' into '{1}'".format(of, build_dir))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are py36+ so we can use f-strings 😄

@tacaswell tacaswell force-pushed the plot_directive_preserve branch from e222429 to e238368 Compare August 12, 2019 00:34
@tacaswell
Copy link
Member

sorry, I read the year when this was opened wrong and thought it was from this January, not from 2 years ago.

@tacaswell tacaswell dismissed their stale review August 12, 2019 00:43

I did the re-base and added the leading spaces

@tacaswell tacaswell requested a review from alexrudy August 12, 2019 00:43
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modulo tests passing

@tacaswell tacaswell force-pushed the plot_directive_preserve branch from e9fe4f7 to 725f25d Compare August 19, 2019 19:39
@tacaswell tacaswell removed the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Sep 4, 2019
@tacaswell tacaswell modified the milestones: v3.2.0, v3.3.0 Sep 4, 2019
@timhoffm timhoffm force-pushed the plot_directive_preserve branch from 2c10b86 to 134aaf9 Compare November 24, 2019 18:12
@timhoffm
Copy link
Member

I took the liberty of pushing an update to fix the doc issues.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a rebase on master. Then move whats new entry to our new whats new scheme.

@@ -519,7 +548,7 @@ def get_plot_formats(config):

def render_figures(code, code_path, output_dir, output_base, context,
function_name, config, context_reset=False,
close_figs=False):
close_figs=False, outname=''):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would go with outname=None, which is IMHO a more standard way for "not defined" than an empty string.

@@ -0,0 +1,61 @@
Plot Directive `outname` and `plot_preserve_dir`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to rename plot_preserve_dir to plot_cache_dir.

@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 1, 2020
@jklymak jklymak marked this pull request as draft August 24, 2020 14:20
@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021
@QuLogic QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 23, 2021
@timhoffm timhoffm modified the milestones: v3.6.0, unassigned Apr 30, 2022
@story645 story645 modified the milestones: unassigned, needs sorting Oct 6, 2022
@dstansby
Copy link
Member

See #25091 (comment) for context on closing.

@dstansby dstansby closed this Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants