Add an output-base-name option to the Sphinx plot directive #28187
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
| format ``{counter}`` to use an incremented counter. For example, | ||
| ``'plot-{counter}'`` will create files like ``plot-1.png``, ``plot-2.png``, | ||
| and so on. If the ``{counter}`` is not provided, two plots with the same | ||
| output-base-name may overwrite each other. |
There was a problem hiding this comment.
Perhaps the extension should actually check for this and just error.
There was a problem hiding this comment.
OK, nevermind, I undid this change. This seems harder to do than I thought, because you'd need to keep track of which files are generated.
|
For anyone interested, the error you get if you try to use the plot directive with MyST directly is |
|
Ah, I didn't realize there actually already are unit tests for this. Let me update that. |
This allows specifying the output base name of the generated image files. The
name can include '{counter}', which is automatically string formatted to an
incrementing counter. The default if it is not specified is left intact as
the current behavior, which is to use the base name of the provided script or
the RST document.
This is required to use the plot directive with MyST, because the directive is
broken with MyST (an issue I don't want to fix), requiring the use of
eval-rst. But the way eval-rst works, the incrementing counter is not
maintained across different eval-rst directives, meaning if you try to include
multiple of them in the same document, the images will overwrite each other.
This allows you to manually work around this with something like
```{eval-rst}
.. plot::
:output-base-name: plot-1
...
```
```{eval-rst}
.. plot::
:output-base-name: plot-2
...
```
Aside from this, it's generally useful to be able to specify the image name
used for a plot, as a more informative name can be used rather than just
'<document-name>-1.png'.
|
For me, the Sphinx extension tests fail on main |
|
I added a test that will hopefully work. I'm having difficulty getting some of the tests to run successfully locally right now, but I believe the asserts I added work. If CI fails I might need some help figuring out why things aren't working on my computer. |
| else: | ||
| source_file_name = rst_file | ||
| code = textwrap.dedent("\n".join(map(str, content))) | ||
| counter = document.attributes.get('_plot_counter', 0) + 1 | ||
| document.attributes['_plot_counter'] = counter | ||
| base, ext = os.path.splitext(os.path.basename(source_file_name)) | ||
| output_base = '%s-%d.py' % (base, counter) | ||
| if options['output-base-name']: | ||
| output_base = options['output-base-name'].format(counter=counter) |
There was a problem hiding this comment.
could we prefix in the script/rst file name here as well? If you have duplicate output names in the same rst file that is a quick search to find and fix, but if you have the collision across multiple rst files it could be much more annoying to find where they are colliding.
There was a problem hiding this comment.
Well my thinking here is also that users should have full control over the filename that is produced. If we always inject something into the name, that won't be the case. For instance, if you right-click and save a plot, this will be the filename used for the image.
Really, we need to figure out how to error if the same name is used twice. My naive idea of checking if the file already exists doesn't work because it could just exist from a previous build. It's probably possible to keep a global list of already used filenames, but I'll have to figure out how to make that work with partial Sphinx rebuilds.
|
What version of pytest are you using? I could not reproduce this issue last night. |
|
I am 👍 on this idea, one small question about the naming (I see both sides of forcing an additional prefix or not). I re-milestoned this for 3.10 as it is a new feature which seems too big to put in via a micro release and it is too late to get this is under the wire for 3.9. |
It's possible my local build of matplotlib is broken somehow. pytest also does a really annoying thing when I run this test on my Mac where it switches to another space like it is opening a GUI window. I also had to run or the tests would just error completely with some strange error (which fortunately ChatGPT was able to figure out). I tried my best to follow your dev instructions, but some of the steps didn't work (e.g., the dev requirements.txt doesn't actually list any build requirements). |
|
The reason I suspect pytest is this path: What was the strange error? I'm really not sure why you need X11 stuff for the tests of the sphinx extension to run... We have an open PR to fix the dev install instructions. The conda environment file works, most of the regular devs who don't use conda know what to do, and we explicitly install them on CI so it fell through the cracks. |
|
Sorry that I wasn't clear. I reproduced the Sphinx issue outside of pytest, using The first output I showed is from pytest (which is running that command). The second is the log file from running the command directly, which shows the full traceback. |
|
Ah, so looking closer at https://matplotlib.org/devdocs/devel/development_setup.html#create-a-dedicated-environment, I see there is a secret "conda" button you have to click. I really don't like that Sphinx extension that hides content behind tabs... |
|
oh, I bet our tiny pages config needs the backend set to |
|
The extension supposedly does that But perhaps my private matplotlib config is messing with things. |
|
I have some work-in-progress code to do the duplicate filename checks. I'm trying to figure out why this matplotlib/lib/matplotlib/sphinxext/plot_directive.py Lines 484 to 490 in eb17273 |
|
By the way, I saw you approved this. If you want to merge this as-is, that's fine. I can add the duplicate filename check in a separate PR. I'm still not completely sure yet if I can actually get it working. |
|
So it occurs to me that this might actually do unexpected things if a global output_base_name is set when there are partial rebuilds. I think I might just remove the ability to have a global option, as well as the |
|
This is also going to do funny looking things if you mix plots with and without set base names as the counter is always increased so I think you will get names like [ One option would be to the Another option would be to instead of thinking of this as setting the base name, change the user facing knob to be a postfix so either we append a counter OR we stick on what ever the user gave us. I think that will fix the myst problem and prevents inter-document name collisions. |
tacaswell
dismissed
their stale review
Too enthusiastic, still some open questions.
|
My plan is to simplify this considerably:
|
|
You can have multiple figures in one matplotlib/lib/matplotlib/tests/tinypages/some_plots.rst Lines 161 to 168 in 955432c |
This allows specifying the output base name of the generated image files. The name can include '{counter}', which is automatically string formatted to an incrementing counter. The default if it is not specified is left intact as the current behavior, which is to use the base name of the provided script or the RST document.
This is required to use the plot directive with MyST, because the directive is broken with MyST (an issue I don't want to fix), requiring the use of eval-rst. But the way eval-rst works, the incrementing counter is not maintained across different eval-rst directives, meaning if you try to include multiple of them in the same document, the images will overwrite each other. This allows you to manually work around this with something like
Aside from this, it's generally useful to be able to specify the image name used for a plot, as a more informative name can be used rather than just
<document-name>-1.png.Note: I've done minimal testing of this, but more extensive testing would probably be a good idea.
PR summary
PR checklist