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

[MRG+1] IPython notebook parser #75

Merged
merged 14 commits into from Jan 15, 2016

Conversation

Projects
None yet
4 participants
@Titan-C
Member

Titan-C commented Dec 7, 2015

This PR addresses #39
Using the block splitting of the scripts introduced for the notebook styled example. It is now simple to construct an IPython notebook cell by cell.
I now include as a dependency the nbformat which defines the cell nodes of the Ipython notebook.
Code is tidy in the notebooks. The RST text is understood by the Notebook quite fine.

Now that I see on travis python2.6 fails

@@ -3,3 +3,4 @@ matplotlib
pillow
sphinx
nose
nbformat

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Dec 7, 2015

Contributor

nbformat pulls in traitlets, ipython_genutils, jsonchema, and jupyter_core.

I worry particularly about jupyter_core: we could easily run into conflicts or break an IPython install by dragging in an incompatible version.

It seems like a big cost, given that writing a notebook is not much more than writing a json file. How difficult would it be to have our own code for writing such a file?

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Dec 7, 2015

Contributor

The failure on 2.6 is that traitlets is not 2.6 compatible.

This comment has been minimized.

@Titan-C

Titan-C Dec 7, 2015

Member

nbformat pulls in traitlets, ipython_genutils, jsonchema, and jupyter_core.

They are indeed a lot of dependencies, it's too much

It seems like a big cost, given that writing a notebook is not much more than writing a json file. How difficult would it be to have our own code for writing such a file?

Not difficult at all. I started with nbformat, because that is the library for the notebook file and it provides their utilities. Now, I just used python standard json library. The file is not too ugly after enforcing indentation on save(Now that anyone might be willing to read it). It does not come out nicely ordered as IPython seems to prefer. But It the file is a valid JSON file and IPython seems to open it without problems. Upon saving it from the notebook it does recover the nice ordering.

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux via email Dec 7, 2015

Contributor
@Titan-C

This comment has been minimized.

Member

Titan-C commented Dec 7, 2015

I have added an extra converter from rst to IPython markdown. Well it only parses the top heading and math. Other headings as they are not overlined work well.

The boundary case syntax used in the plot_parse example is not parsed. There are exotic undelines characters for headings, and heavy use of rst syntax. Is just rendered as text. For the rest of the examples in here they look nice

@Titan-C Titan-C changed the title from [WIP] IPython notebook parser to [MRG] IPython notebook parser Dec 17, 2015

@Titan-C Titan-C force-pushed the Titan-C:notebook branch from e11cf34 to 7fc8317 Dec 29, 2015

@Titan-C Titan-C added this to the Release v0.0.12 milestone Dec 29, 2015

@Titan-C Titan-C force-pushed the Titan-C:notebook branch from 00a19b3 to 838c304 Jan 10, 2016

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jan 10, 2016

Rebased to master, conflicts solved. Reviews needed

text = re.sub(inline_math, r'$\1$', text)
return text

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Jan 10, 2016

Contributor

For PEP8 compatibility, you should have an extra empty line here (two empty lines between top-level blocks).

@@ -8,8 +8,10 @@
import tempfile
import sphinx_gallery.gen_rst as sg
import sphinx_gallery.notebook as gnb

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Jan 10, 2016

Contributor

In general, those shorthands make the code harder to read. In my world, GNB means "Gaussian Naive Bayes" :).

Maybe you could simply use "from sphinx_gallery import notebook".

example_nb.save_file()
f.flush()
assert_equal(gnb.json.load(f), example_nb.work_notebook)

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Jan 10, 2016

Contributor

It would be good to directly import json, and not use the one that is inherited from gnb. The reason is that, to the reader that doesn't know the entire codebase, it is not clear at all that "gnb.json" is the json module from the stdlib.

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Jan 10, 2016

Overall the code is nice, and it is really cool that you got math working! Scikit-image needed that for integration of the gallery.

I made a few cosmetic comments, but aside these, 👍 for merge.

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jan 12, 2016

This needs a rebase actually.

@Titan-C Titan-C force-pushed the Titan-C:notebook branch from 838c304 to 43bcfaa Jan 14, 2016

@Titan-C Titan-C force-pushed the Titan-C:notebook branch from 67cfdfe to f712cff Jan 14, 2016

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jan 14, 2016

Rebased to master and comments addressed.

@GaelVaroquaux GaelVaroquaux changed the title from [MRG] IPython notebook parser to [MRG+1] IPython notebook parser Jan 15, 2016

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Jan 15, 2016

LGTM. +1 for merge.

@agramfort

This comment has been minimized.

Contributor

agramfort commented Jan 15, 2016

works like a charm !

+1 for merge.

really nice @Titan-C !

GaelVaroquaux added a commit that referenced this pull request Jan 15, 2016

Merge pull request #75 from Titan-C/notebook
[MRG+1] IPython notebook parser

@GaelVaroquaux GaelVaroquaux merged commit f3859b0 into sphinx-gallery:master Jan 15, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Jan 15, 2016

🍻 Yey! yey! yey!

@agramfort

This comment has been minimized.

Contributor

agramfort commented Jan 15, 2016

🍻 !!!

@Titan-C Titan-C deleted the Titan-C:notebook branch Feb 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment