-
-
Notifications
You must be signed in to change notification settings - Fork 8k
sphinx plot directive: sources relative to rst file #12456
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
Conversation
This needs at least a note in the api_changes explaining the change and how people relying on the old behavior need to update their code.
In practice I think(?) "absolute" paths should still be detected and understood relative to srcdir; this is consistent with how sphinx treats
:doc:`/absolute/path`
In practice I think(?) "absolute" paths should still be detected and understood relative to srcdir
What happens when you have both an "absolute" path and have also set the plot_basedir
option as well?
Uh.
I guess the following behavior would have been reasonable, and covers all use cases relatively easily, with a relatively simple upgrade path (just use absolute paths):
- treat empty plot_basedir as plot_basedir = srcdir
- treat relative paths as relative to the current file's directory
- treat absolute paths as relative to plot_basedir
But that's neither what was previously used, nor what was previously documented. Moreover, this means that docs won't be buildable with both old and new versions of mpl. (The design proposed in the PR has the same issue I think?)
One standard-ish way to handle the transition period would be to introduce a global plot_path_resolution_method = "old" / "new" (names up to bikeshedding) in conf.py and switch the behavior based on that, then we can add deprecation for the "old" method and default to "new". A bit painful, but heh...
I don't know where to put the API changes text so I'll just post it here for now:
Fixed a bug in the sphinx plot directive (.. plot:: path/to/plot_script.py)
where the source Python file was not being found relative to the directory of
the file containing the directive.
Documents that were using this feature may need to adjust the path argument
given to the plot directive. Two options are available:
1. Use absolute paths to find the file relative the ``plot_basedir`` (which
defaults to the directory where conf.py is).
2. Use relative paths and the file is found relative to the directory of the
file containing the directive.
I have done the new method here and provided fixes to the broken documentation. I'm not sure if I can implement the deprecation mechanism.
The api changes note needs to go to this directory: https://github.com/matplotlib/matplotlib/tree/master/doc/api/next_api_changes.
I'm fine with the change but let's give other devs the time to chime in especially re: behavior change.
d47c054
to
6b14b00
Compare
I am a bit concerned about this, I expect it to break the docs of a fair number of down-stream libraries that use the plot directive as if they are using paths at all they will need to be changed. Atleast scipy and basemap use it (ex
(bleeding) ✔ ~/source
20:42 $ ack --rst ".. plot:: "
other_source/scipy/doc/source/tutorial/stats.rst
592:.. plot:: tutorial/examples/normdiscr_plot1.py
597:.. plot:: tutorial/examples/normdiscr_plot2.py
953:.. plot:: tutorial/stats/plots/kde_plot2.py
967:.. plot:: tutorial/stats/plots/kde_plot3.py
1010:.. plot:: tutorial/stats/plots/kde_plot4.py
1064:.. plot:: tutorial/stats/plots/kde_plot5.py
I understand and I think having a deprecation period and being able to specify the old and new method is important here (as suggested).
Not sure if I see this being resolved before 3.1. Not quite sure I understand why this isn't simply a documentation issue...
@jklymak this is an implementation issue as explained #10350 and we have a fix for it here. The problem is this fix is going to break a lot of packages that depended on this bug (even matplotlib itself). So we'll have to have a deprecation period and support both methods for a while and then remove the old method after a while. Now, I have implemented the new method but was hoping a maintainer would take it from here on and implement the deprecation.
According to the documentation when plot_basedir is empty or None. The source-code files are found relative to the document that contains them. However, this was not true in the actual implementation. Fixes matplotlib#10350
Absolute paths are found relative plot_basedir which defaults to the source directory. Relative paths are found relative to the document containing them.
2933c52
to
7ceea9e
Compare
@tacaswell Can you make the call regarding the API change? I think the new behavior is nicer, but will be a bit disruptive for downstream projects.
I agree that an absolute path out to the host file system is probably not what anyone wants.
We may want to add a "old-but-dont-warn" option. For down-stream projects they will want to be able to build their docs with a range of mpl versions, at least for a while.
Maybe rename the two modes to file-relative
and root-relative
? They are more descriptive and less judgmental.
I am 👍 on moving our docs to use file-relative
.
How does this interplay (if at all) with #13858?
Converting to draft. This seems like it will not get fixed, but rather we should properly document the existing behaviour?
PR Summary
According to the documentation when plot_basedir is empty or None.
The source-code files are found relative to the document that contains
them. However, this was not true in the actual implementation.
Fixes #10350
PR Checklist