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

Update IPython directive #4570

Merged
merged 16 commits into from Dec 19, 2013
Merged

Update IPython directive #4570

merged 16 commits into from Dec 19, 2013

Conversation

chebee7i
Copy link
Contributor

Here are a number of updates to the IPython directive:

  • matplotlib is now optional (but on by default). This is configurable through an extension parameter ipython_mplbackend. The backend is also configurable, default is 'agg'.

  • The intent of the directive was to automatically add NumPy and pylab to the namespace. This broke recently, but has been fixed. It is also configurable now via the extension parameter ipython_execlines. Would it be okay to update the default to include pyplot?

    ['import numpy as np', 'import matplotlib.pyplot as plt', 'from pylab import *']
    
  • For @doctests within the IPython directive, it can be helpful to fuzzy compare for equality of output. Even if the calculation is performed with a known seed (in the case of stochastic dependence), if the test was written on a slightly different version of software, there could be very small differences which would cause the test to fail. The user can now do:

    @doctest float
    In [1]: 0.1 + 0.2
    Out[1]: 0.3
    

And this will count as a valid test. The rendered output will display: 0.30000000000000004. A 2nd and 3rd argument specify the rtol and atol passed to np.allclose (both are required if specified at all). This handles infs, nans, higher dimensional arrays, etc. in very basic scenarios---those you'd expect to run across with documentation examples.

Some additional documentation is still needed, and there's another bug I've found that still needs fixing.

@chebee7i
Copy link
Contributor Author

For this PR and also for #4549, it would be nice to hear from @matplotlib that their docs build and are highlighted correctly still. I've done some testing, but they aren't nearly as comprehensive.

@takluyver
Copy link
Member

Pinging @mdboom @ivanov to test this with the matplotlib docs.

@Carreau
Copy link
Member

Carreau commented Dec 5, 2013

Should we re-ping matplotlib ? open an issue on their side so that they check ?

@damianavila
Copy link
Member

Should we re-ping matplotlib ? open an issue on their side so that they check ?

Probably the second option, to get more attention...

@mdboom
Copy link
Contributor

mdboom commented Dec 5, 2013

I'm not sure I follow. matplotlib's documentation does not use the ipython_directive. It uses the older/simpler ipython_console_highlighting plugin (included with matplotlib) that does nothing but the syntax highlighting part.

@Carreau
Copy link
Member

Carreau commented Dec 6, 2013

You do seem to still have ipython_directive and #3352 and related was done based on your suggestion. So I thought you were using IPython directive if IPython is availlable.

@mdboom
Copy link
Contributor

mdboom commented Dec 6, 2013

Sorry, folks, and thanks for the reminder. Not enough coffee yesterday. I'll look into implementing using IPython's directives if they are available and see what, if anything, breaks.

@mdboom
Copy link
Contributor

mdboom commented Dec 6, 2013

Yes, matplotlib still includes the ipython_directive (and it's probably time to deprecate and then remove it there), but it is not used to build our own docs (never has been, as far as I know), so I think this PR is irrelevant as far as checking against matplotlib.

@Carreau
Copy link
Member

Carreau commented Dec 6, 2013

OK, great thanks, no problem for the reminder I had to dig into issues to find it myself.
So I guess we can go ahead with this one and have further fix later on if there are issues on matplotlib side.
I'll let that open a few more hours for other dev to have a look.

@mdboom
Copy link
Contributor

mdboom commented Dec 6, 2013

Related -- this is a PR to use IPython's ipython_console_highlighting extension, if it's available:

matplotlib/matplotlib#2656

@takluyver
Copy link
Member

pandas is using the IPython directive in their docs if we want someone to test it.

@Carreau
Copy link
Member

Carreau commented Dec 6, 2013

pandas is using the IPython directive in their docs if we want someone to test it.

So we should ping @wesm I guess. (BTW, Wes if you read this, I had to export some data for biologists, I love you for having a DataFrame.to_excel)

@takluyver
Copy link
Member

I think other people do a lot of the development on pandas now, but I'm not sure who's most relevant. Maybe @jorisvandenbossche , who updated ipython_directive a few months ago. Or maybe one of us should just grab the pandas repo, substitute in the IPython directive extension from this PR, and try building the docs. I have a checkout of pandas, I can probably give that a try later.

@jorisvandenbossche
Copy link
Contributor

I can try it later this weekend to build the pandas docs with this, but at the moment we are not using the IPython's sphinx extension itself, but an older and in the meanwhile slightly adapted version included in pandas source. See pandas-dev/pandas#5221 for some discussion about using IPython's instead our own version. For most of the docs this already works (eg not for the cython doc page, as for this the directive was adapted). So I can give it a try.

@takluyver
Copy link
Member

Thanks Joris. If it's not too difficult to merge pandas' changes with the changes here and check that it still builds OK, that would be good.

@jorisvandenbossche
Copy link
Contributor

I tried it out, and I don't see more problems with this PR than with version in master :-)
To say, there are some issues with using the IPython's version, but that is both for master and this PR. I summarised them at the issue pandas-dev/pandas#5221, and they seem not to difficult to merge them here.

So for me, this PR is OK (but I didn't really look at the code and I didn't test the doctest part, as this is not used in pandas). However I have the following remark:

  • regarding the default imports in ipython_execlines. Is it necessary to have a default value? And more particularly the pylab one from pylab import *? For some reason this seems to screw up a part of our docs because pylab imports the datetime module. Off course we can set this to an empty list, but I think more in general from pylab import * should not be encouraged?
  • The usage of default / None seems different for mplbackend and execlines: For ipython_mplbackend setting it at None imports nothing (while default is 'agg'), while for ipython_execlines setting this at None imports the default. Is this None necessary for ipython_execlines? Why not simply have: when not specified in conf.py default to the default ['import ...'], otherwise use the one listed in conf.py?

@chebee7i
Copy link
Contributor Author

I'll take a look at pandas-dev/pandas#5221 in the next day or so. There are also a few other things I'd like to do before this is merged (holidays have distracted me).

Regarding the options, I'm happy to change them to whatever is decided upon. My rationale for having the import * was only because it was in there already. I'd be just as happy setting the default to an empty list, but if we are to have a default, then I'm definitely in agreement on the starred-import: it should be discouraged. My suggestion for import matplotlib.pyplot as plt was a compromise...but I'd really like to remove the import * statement altogether.

As for defaults, that's a good idea. I'll make the change. Also, I'll probably make a non-backend require the string 'none', and change it so that None (the singleton) will keep the default.

…xeclines.

When specified as `None` in conf.py, this is treated the same as if not
specified in conf.py at all.
@takluyver
Copy link
Member

If this and #4549 are still open on Monday, I'll do another quick sanity check on them, then take a merge-em-and-see-who-complains approach. If anyone disagrees, you've got the weekend to let me know. ;-)

@minrk
Copy link
Member

minrk commented Dec 14, 2013

👍 to both

@damianavila
Copy link
Member

👍 too


return savefig_dir, source_dir, rgxin, rgxout, promptin, promptout
if mplbackend is None:
mplbackend = 'agg'
Copy link
Contributor

Choose a reason for hiding this comment

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

In the explanation of ipython_mplbackend, you now state that if None, nothing is imported. Does this not contradict with this? Or do I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opps. That shouldn't be there as of now, but given @mdboom's comment, something like the above will probably stay.

@jorisvandenbossche
Copy link
Contributor

I tested it, and it seems good!

The issue with the ' Out[]' prompt, I am also seeing that. However, this also occurs with ipython master, so has not to do with your changes in this PR. But it still needs to be solved of course.
The strange thing is, when I build the pandas docs, I see one file where the Out prompts are as expected (but I don't see why this file would be different) and in all the other files, it is only in the beginning, eg ' Out[10]', but once the prompt number is above 100, it is ok ('Out[100]'). Maybe this helps.

@chebee7i
Copy link
Contributor Author

Yeah its a strange bug. Let's punt on that for now and I'll file a bug report.

@takluyver
Copy link
Member

I wonder if something is adding the spaces to line up the Out[n] prompts of different widths. @jorisvandenbossche , is the one file that works OK one where there are no Out prompts higher than Out[9], for instance?

@@ -0,0 +1,108 @@
"""
Module containing custom doctests.
Copy link
Member

Choose a reason for hiding this comment

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

'Module containing' is superfluous, but in what way are the doctests custom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in d9d261d

@takluyver
Copy link
Member

Checked the diff as of 311596d, and there's nothing obviously amiss.

@chebee7i
Copy link
Contributor Author

Here is a document that keeps n below 10 but still has the problem:

http://docs.dit.io/en/latest/measures/total_correlation.html

Here is another that lets n go above 10 but does not have the problem:

http://docs.dit.io/en/latest/generalinfo.html

If I take out some documents from the build process, documents which had the problem cease to have the problem. Def need a minimal example.

@jorisvandenbossche
Copy link
Contributor

@takluyver No, it was with the 10min page of the pandas docs, so prompt numbering goes even above hundred. What is special about this page, is that is the first page that is build by sphinx (that's the only thing I can think of at the moment). @chebee7i The generalinfo page which also does not have the problem, is also the first page with ipython directives in it that is built I think, is that correct?

@takluyver
Copy link
Member

Ah, I bet the prompt numbering is being reset, but not the prompt justification - so if a previous page has gone to Out[10], the next page to be built will line up the Out[n] prompts with the right hand side of that (and likewise if a page reaches Out[100]).

@chebee7i : do you still want to leave that for another PR, or try to fix it in this?

@jorisvandenbossche
Copy link
Contributor

Could it maybe something like this: Once the prompt number has been above 100 (for pandas docs, or above 10 for dit), and in our cases this already happens at the first page which is built, 'it' (I don't know exaxtly what) remembers the width of the Out[] prompt and retains it for the next page, although the prompt is less wide as the numbers start at 1 again.

This seems a little bit logical with what I see in the results, however I totally don't know how this could be explained technically. As it just captures the shells stdout, I don't see how this could happen, but I am not really familiar with this.

@jorisvandenbossche
Copy link
Contributor

@takluyver Ah, I didn't see your new comment. But seems I was thinking in the right direction :-)

@takluyver
Copy link
Member

The relevant bit of code is here:

https://github.com/ipython/ipython/blob/master/IPython/core/prompts.py#L436

So I guess that, when you reset the execution count, you also need to reset IP.prompt_manager.width. I think setting it to 0 will work - it will take its width from the next prompt it produces.

@jorisvandenbossche
Copy link
Contributor

I can confirm this does the trick!

@jorisvandenbossche
Copy link
Contributor

This commit jorisvandenbossche@1fbc5f6 solves the issue (@chebee7i I did sent it as a PR to your branch, I don't know exactly how it works).

Set prompt_manager.width to zero for each new document to prevent misaligned Out prompts
@chebee7i
Copy link
Contributor Author

Nice job on finding the fix!

I merged it and also added a new option ipython_holdcount. When True (the default), inputs preceded by @suppress will not increment the execution count.

@takluyver
Copy link
Member

Thanks @chebee7i and @jorisvandenbossche . I'm going to merge this. Feel free to file more pull requests if you find problems.

takluyver added a commit that referenced this pull request Dec 19, 2013
@takluyver takluyver merged commit 95829b6 into ipython:master Dec 19, 2013
@jorisvandenbossche
Copy link
Contributor

@chebee7i Are you working on some of these things? I maybe have some time to try to get the okexcept stuff in before 1.2/2.0, but just checking to avoid double work.

@chebee7i
Copy link
Contributor Author

@jorisvandenbossche not currently working on it. So if you have the time, go for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants