Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add an image-basename option to the Sphinx plot directive#28187
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
base:main
Are you sure you want to change the base?
Conversation
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.
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 uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Perhaps the extension should actually check for this and just error.
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.
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. Thename can include '{counter}', which is automatically string formatted to anincrementing counter. The default if it is not specified is left intact asthe current behavior, which is to use the base name of the provided script orthe RST document.This is required to use the plot directive with MyST, because the directive isbroken with MyST (an issue I don't want to fix), requiring the use ofeval-rst. But the way eval-rst works, the incrementing counter is notmaintained across different eval-rst directives, meaning if you try to includemultiple 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 nameused 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. |
asmeurer commentedMay 9, 2024 • 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.
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 athttps://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. |
asmeurer commentedMay 9, 2024 • 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 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 ineb17273
|
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 counterOR we stick on what ever the user gave us. I think that will fix the myst problem and prevents inter-document name collisions. |
My plan is to simplify this considerably:
|
| @@ -174,3 +174,18 @@ Plot 21 is generated via an include directive: | |||
| Plot 22 uses a different specific function in a file with plot commands: | |||
| .. plot:: range6.py range10 | |||
| Plots 23 through 25 use output-base-name. | |||
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.
Why do we need 3? / What do we get more compared to one plote?
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.
The second one probably isn't needed (it was when I had the counter thing but I removed that). The third one is testing the case when the plot comes from a file, which is a completely separate code path in the extension.
| @@ -47,6 +47,12 @@ | |||
| The ``.. plot::`` directive supports the following options: | |||
| ``:output-base-name:`` : str | |||
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.
Thanks for the clarification. In that case*-base-name makes sense.
Two further thoughts:
- would
image-base-namebe better (because more concrete) thanoutput-base-name? - what happens / do we need to check and error if the user tries to give more than a base name, e.g.
:output-base-name: ../escaped_from_image_dir/myimageornested_dir/myimage?
I'm definitely open to a better option name here. I mostly just copied what it was called in the code, but we should try to find something that's obvious to end users. |
Good question. I'll need to test it. Actually a more common error would be trying to add the extension to the base name, like |
Yes, extensions should not be allowed. Possibly slightly more restrictive than need be, but for simplicity I'd go with not allowing |
I made those changes. Still wondering if there are any better suggestions for the option name than |
timhoffm commentedOct 18, 2024 • 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'd go with |
OK, I've renamed |
| del env.mpl_custom_base_names[docname] | ||
| def merge_other(self, app, env, docnames, other): | ||
| for docname in docnames: |
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.
Do we need to loop over the docnames? AFAICS, we only want to mergeother.mpl_custom_base_names intoenv.mpl_custom_base_names. I.e. we can directly loop over that.
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.
OK, I simplified this function. I'm not completely sure about it, but the functionality still seems to work based on manual testing.
| for docname in docnames: | ||
| if docname in other.mpl_custom_base_names: | ||
| if docname not in env.mpl_custom_base_names: | ||
| env.mpl_custom_base_names[docname] = set() |
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.
Can be left out because mpl_custom_base_names is a defaultdict.
| def init_filename_registry(app): | ||
| env = app.builder.env | ||
| if not hasattr(env, 'mpl_custom_base_names'): | ||
| env.mpl_custom_base_names = defaultdict(set) |
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.
The name is a bit generic. Maybe
| env.mpl_custom_base_names=defaultdict(set) | |
| env.mpl_plot_image_basenames=defaultdict(set) |
Or similar. This naming islibrary_directive_option.
I think the docs build error isn't my fault this time, from what I can tell. |
Any more thoughts on this? |
We just need a second core developer to approve. |
@tacaswell you looked at this previously. Do you have any thoughts? |
Can this be merged? It's been sitting for some time without any further review. |
We still need a second review from a core developer. |
|
If you're talking about |
Is this CI test failure related to my changes? |
No it's a flaky test. |
Uh oh!
There was an error while loading.Please reload this page.
This allows specifying the output base name of the generated image files.
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.PR summary
PR checklist