Skip to content
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

sphinxext/plot_directive does not accept a caption #4702

Merged
merged 8 commits into from Dec 8, 2015

Conversation

hamogu
Copy link
Contributor

@hamogu hamogu commented Jul 17, 2015

According to the docstring I should be able to do

     When a path to a source file is given, the content of the
     directive may optionally contain a caption for the plot::

       .. plot:: path/to/plot.py

          This is the caption for the plot

However, when I try this I get:

Exception occurred:
  File "/Users/hamogu/anaconda/lib/python2.7/site-packages/matplotlib/sphinxext/plot_directive.py", line 640, in run
    raise RuntimeError("plot:: directive can't have both args and content")
RuntimeError: plot:: directive can't have both args and content

The code to generate the caption is there, but because of this line
https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/sphinxext/plot_directive.py#L655
it is impossible to get there.

@mdboom: I'm sure you had a good reason to put this check there when you wrote it, thus I'll leave this as an issue and don't just submit a PR to remove this check.
(Also, I don't know much about how matplotlib runs tests and the code in the doc string should probably be turned into a doctest...)

@mdboom
Copy link
Member

mdboom commented Jul 15, 2015

I can't think of a good reason I had for that check. I'm perfectly fine with it being removed.

@tacaswell tacaswell added this to the proposed next point release milestone Jul 15, 2015
@hamogu
Copy link
Contributor Author

hamogu commented Jul 18, 2015

Does anybody have advice how I make the test (that I added) work?
I did not manage to compile matplotlib locally (I usually use it via anaconda) so I cannot debug this locally.

Alternatively, I can just remove the tests again. It did not have tests before, so we would not be worse off. But if there is a way to make the tests work, then I would obviously prefer that.

@hamogu
Copy link
Contributor Author

hamogu commented Aug 11, 2015

@mdboom If you have any advise how I can debug why my tests fail please let me know.
Alternatively, I can remove the tests again - there were not tests before, so we would not be off any worse (but I'd like to keep them, if I can figure it out).


def range6():
plt.figure()
plt.plot(range(6))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean 4 here, not 6

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or not have this function at all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind, I see the point of this now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that plot 16 executes a specific function in a file. To make sure that works there needs to be some other stuff in the file that is not executed.
However, I might have to protect line 3-5 in some way to make sure they don't get executed on import. Maybe I can just make a function like this

def nevercalled():
    raise NotImplementedError

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense

@WeatherGod
Copy link
Member

I don't have time to run this branch at the moment, but looking at the test error:

======================================================================
ERROR: matplotlib.sphinxext.tests.test_tinypages.TestTinyPages.test_some_plots
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/matplotlib-1.5.dev1-py2.7-linux-x86_64.egg/matplotlib/sphinxext/tests/test_tinypages.py", line 87, in test_some_plots
    assert_true(file_same(range_4, pjoin(self.html_dir, 'range4.png')))
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/matplotlib-1.5.dev1-py2.7-linux-x86_64.egg/matplotlib/sphinxext/tests/test_tinypages.py", line 30, in file_same
    with open(file2, 'rb') as fobj:
IOError: [Errno 2] No such file or directory: '/tmp/tmpGeNmDd/html/range4.png'

it makes me wonder if the plot directive just simply didn't work (or run?).

@hamogu
Copy link
Contributor Author

hamogu commented Aug 11, 2015

I suspect that the file name is different, but I cannot tell without running this locally, which I cannot do since I apparently failed to install all the dependencies need to compile matplotlib from scratch.

@WeatherGod
Copy link
Member

I'll give it a try later today.

@hamogu
Copy link
Contributor Author

hamogu commented Sep 18, 2015

Can anybody who knows how to compile matplotlib from source please try this and see why the tests fail?
I suspect that the filenames of the test images are not what I think they are, but I do not know how to compile matplotlib from source, so I cannot try this myself.

@tacaswell
Copy link
Member

@hamogu You should learn to compile mpl! It is very hard to develop if you can not actually test your code.

@Tillsten
Copy link
Contributor

@tacswell on windows it is really hard :(

@hamogu
Copy link
Contributor Author

hamogu commented Sep 19, 2015

I appreciate your advice about learning to compile mpl in order to contribute to the development. I promise that I devoted and entire afternoon to set up the dependencies and I still failed. Please let me explain why I decided to stop at that point; maybe I can convince you that I'm not just trying to be difficult.

@tacaswell : I'm not planning on contributing to mpl regularly. I am a regular contributor to astropy and I have some other astronomy projects in python and C (some of which I'm even paid for). My background is in astronomy and I'd rather concentrate on a few projects, where I can have an impact, instead spending my time to learn each and every build system out there (additionally, my wife just had a baby).

@Tillsten : I'm on linux (CentOS) or Mac OS,, which should work in principle, but CentOS generally is not very up-to-date in terms of packages in the repositories and as an added complication, I don't have full root access to that machine. Thus, I use mpl through a binary installer (ananconda).

Thus, for projects that I'm not familiar with, I'll generally only contribute trivial PRs (and the code change here is trivial and may have been accepted long ago, if I had not tried to also add the tests that complicate the matter).

So, unfortunately, that means that this PR will stay unfinished, unless I find somebody to help me out in debugging this branch.

@tacaswell
Copy link
Member

If you are on linux and using conda, just install mpl into an env, then remove it, and then do the source install into that enviroment (this is how I run my development for the most part). You probably just need a complier from the underlying OS.

anaconda packages are compiled on centOS 5 so it is possible.

@jmehne
Copy link
Contributor

jmehne commented Dec 6, 2015

The test fails, because Plot 15 in some_plots.rst (which calls range4.py) contains code after the first plt.show() call. Thus, matplotlib thinks there are two code pieces/plots to create which changes the file name of the output file.

You can:

  • Change range4.png -> range4_00_00.png. This will make the test pass, but because matplotlib tries to create two plots and the second code piece does not actually create a plot it will add a pair of empty () between range4_00_00.png and the caption in the html output of some_plots.rst. So we should probably add a comment there that explains the beavior.
  • Or, remove everything after the first plt.show() in range4.py. It seems to me that range6.py already tests the same/similar behavior (an additional function definition that should not be called).

If you're not interested anymore in this PR I can also open a new one and just add the patch for one of the options above.

Travis failed with unrelated issues (connection timeout to coveralls).
@hamogu
Copy link
Contributor Author

hamogu commented Dec 8, 2015

All passing now!
Many thanks to @Three-Comrades !

@WeatherGod
Copy link
Member

+1 from me. Great job!

mdboom added a commit that referenced this pull request Dec 8, 2015
sphinxext/plot_directive does not accept a caption
@mdboom mdboom merged commit f3e2fa1 into matplotlib:master Dec 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants