FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sphinx plot directive: sources relative to rst file by 183amir · Pull Request #12456 · matplotlib/matplotlib · GitHub

Repository navigation

sphinx plot directive: sources relative to rst file - #12456

Draft
183amir wants to merge 8 commits into
matplotlib:mainfrom
183amir:patch-1
Draft

183amir wants to merge 8 commits into
matplotlib:mainfrom
183amir:patch-1

Conversation

183amir commented Oct 9, 2018

Copy link
Copy Markdown

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

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • 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

anntzer commented Oct 9, 2018

Copy link
Copy Markdown
Contributor

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`

183amir commented Oct 9, 2018

Copy link
Copy Markdown
Author

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?

anntzer commented Oct 9, 2018

Copy link
Copy Markdown
Contributor

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...

183amir commented Oct 9, 2018

Copy link
Copy Markdown
Author

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.

183amir commented Oct 9, 2018

Copy link
Copy Markdown
Author

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.

anntzer added the API: changes Changes to the public API, typically requiring deprecation. label Oct 9, 2018

anntzer commented Oct 9, 2018

Copy link
Copy Markdown
Contributor

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.

tacaswell added this to the v3.1 milestone Oct 24, 2018

Copy link
Copy Markdown
Member

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

183amir commented Oct 24, 2018

Copy link
Copy Markdown
Author

I understand and I think having a deprecation period and being able to specify the old and new method is important here (as suggested).

jklymak modified the milestones: v3.1.0, v3.2.0 Mar 3, 2019

jklymak commented Mar 3, 2019

Copy link
Copy Markdown
Member

Not sure if I see this being resolved before 3.1. Not quite sure I understand why this isn't simply a documentation issue...

183amir commented Mar 3, 2019

Copy link
Copy Markdown
Author

@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.

183amir and others added 6 commits March 3, 2019 16:12
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.

anntzer commented Mar 17, 2019

Copy link
Copy Markdown
Contributor

@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.

Copy link
Copy Markdown
Member

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.

anntzer commented Apr 3, 2019

Copy link
Copy Markdown
Contributor

How does this interplay (if at all) with #13858?

tacaswell modified the milestones: v3.2.0, v3.3.0 Sep 4, 2019
QuLogic modified the milestones: v3.3.0, v3.4.0 May 2, 2020
jklymak marked this pull request as draft December 20, 2020 19:26

jklymak commented Dec 20, 2020

Copy link
Copy Markdown
Member

Converting to draft. This seems like it will not get fixed, but rather we should properly document the existing behaviour?

QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021
QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 21, 2021
timhoffm modified the milestones: v3.6.0, unassigned Apr 30, 2022
story645 modified the milestones: unassigned, needs sorting Oct 6, 2022

This branch has not been deployed

No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API: changes Changes to the public API, typically requiring deprecation. status: needs rebase status: orphaned PR topic: sphinx extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The sphinx plot directive does not find files relative to the document that contains them.

7 participants


Back | FazBrowse Home | New Git URL