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 a filename-prefix 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
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.
Uh oh!
There was an error while loading.Please reload this page.
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:
|
Uh oh!
There was an error while loading.Please reload this page.
I can't comment on the line directly since it's not part of the diff, but the |
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
I added a test for multiple figures. I can confirm that it does work by testing manually, but for some reason adding leads to a much earlier test failing. |
Good catch on the source link. It is working, but it looks like it isn't properly adding the |
Oh I think that was just because I was testing tinypages locally and it was messing with the tests. |
…docs in tinypagesThe tests now don't copy any of the build files, meaning the tests should passeven if tinypages has stale build state from a manual build.
OK, this is ready for review again. |
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.
Since this also affects the naming of the source file, should we go back tooutput-basename, or are you comfortable with it stayingimage-basename,@timhoffm?
Uh oh!
There was an error while loading.Please reload this page.
I don't care too much between We should consider whetherbasename is appropriate. Whileos.path.basename includes the extension, here it's not included.stem would be the technically correct term. However I feel Either way, I believe the logic is too complex to caputure everything in a single name. Until now, everything was magically determined behind the scenes (i.e. users should not have to bother with the exact name). If we now make the name configurable, we should explain what exactly this is in the associated docs (1) this must not have any path components |
My original version did have something more configurable (the user could specify the format string), but I decided that was way too complex for what was needed here. I think the main thing here is that the user has some way to make the name a little more human readable. If it also adds some numbering or file extensions to that it isn't a problem, especially given that no one else has even raised this as a problem before. And obviously for my original problem the name itself doesn't even matter; it just needs to be unique. |
Then possibly |
FWIW, none of the suggested names really make a ton of sense if you just see them in a |
|
OK, I've updated it to use |
80486e6 intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
Thanks@asmeurer |
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