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.

Sign up for GitHub

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

Jump to bottom

Scraper function for sphinx-gallery, in plotly.io #1577

Merged
merged 17 commits into from
Jul 3, 2019
Merged

Scraper function for sphinx-gallery, in plotly.io #1577

merged 17 commits into from
Jul 3, 2019

Conversation

emmanuelle
Copy link
Contributor

This PR proposes the addition of a scraper function in plotly.io, to be used by the sphinx-gallery sphinx extension when a sphinx-based doc contains a gallery of examples. At the moment sphinx-gallery has a scraper for matplotlib figures, but some open-source projects have expressed their interest in having figures with plotly.py instead.

The PR also contains a minimal sphinx doc with two gallery examples (one for 2D, one for 3D). It's not necessary to merge this but I thought that having gallery examples in the code base could be useful.

Note that this PR does not require a sphinx-gallery dependency, because it will be sphinx-gallery which calls the scraper function and not the other way around.

One thing to decide is whether the function used in examples is plot or iplot. Do most users work in notebooks and therefore use iplot? The function is monkey-patched anyway so it's easy to change.

@emmanuelle
Copy link
Contributor Author

I'm struggling with installing the orca excutable on the CI optional build. Any idea of how to solve this?

@jonmmease
Copy link
Contributor

Hi @emmanuelle, thanks this is very interesting.

A couple of high-level thoughts. Rather than use monkey-patching it would be nice to handle this use-case with a custom renderer (Renderers were introduced in #1474).

I'm also a little hesitant to introduce this logic into the plotly.py repo. Would it make sense to add this to sphinx gallery?

@jonmmease
Copy link
Contributor

I'm struggling with installing the orca excutable on the CI optional build. Any idea of how to solve this?

So far, I've had all of the orca-dependent tests run in the orca builds. These builds install orca using conda

@emmanuelle
Copy link
Contributor Author

Thanks for the comments @jonmmease ! Yes, a custom renderer would be nicer, I just read through #1474 and it seems well adapted.

I initially proposed the scraper function to sphinx-gallery as a PR (sphinx-gallery/sphinx-gallery#493) but their policy is that each plotting library should maintain its own scraper, which kinda makes sense since there is a large number of plotting libraries. So if we want that other projects use plotly in their documentation when using sphinx-gallery (it was mentioned on the scikit-learn mailing-list that there is interest for this), I think we have to include it and maintain it...

@jonmmease
Copy link
Contributor

Thanks for the background @emmanuelle, having something in the plotly.py repo is fine in that case. Looking over the code, I think we could replace the _patched_plotly_plot logic with a custom renderer.

Would it be possible to get away with only having a custom renderer and not the plotly_sg_scraper and figure_rst logic? Can we make the output of plotly look just like matplotlib so that scraper built into sphinx gallery picks it up? It doesn't look like there's anything in these two functions that's specific to how plotly.py works, so it would be nice to avoid them if possible. Thanks!

@emmanuelle
Copy link
Contributor Author

Thanks @jonmmease. The way sphing-gallery handles matplotlib examples is quite different, because it loops through the list of existing matplotlib figures with plt.get_fignums (https://github.com/sphinx-gallery/sphinx-gallery/blob/master/sphinx_gallery/scrapers.py#L87), and we don't have such a list in plotly.py if I understand well. Therefore for plotly figures what I did instead was to save the figure as files and then move the files to the right location. And the figure_rst looks very much like the figure_rst of sphinx-gallery except that it's html and not a png file so another rst directive is needed. But I could ask the sphinx-gallery team if they would accept a PR for extending their figure_rst function to other formats, they didn't want to maintain the scrapers but a figure_rst function with html support might be ok for them.

@emmanuelle emmanuelle mentioned this pull request May 30, 2019
Closed
@emmanuelle
Copy link
Contributor Author

@jonmmease here is a version with a custom renderer. I can add more documentation and comments but first tell me whether this was what you had in mind. I also changed the examples so that now they call plotly.io.show() since this is the syntax we'll be pushing now.

@emmanuelle
Copy link
Contributor Author

Oh and the tests failure seem to be due to something else than the tests added by this PR.

jonmmease
jonmmease reviewed Jun 1, 2019
View reviewed changes
plotly/io/_renderers.py Outdated Show resolved Hide resolved
jonmmease
jonmmease reviewed Jun 1, 2019
View reviewed changes
plotly/io/_sg_scraper.py Outdated
# This module defines an image scraper for sphinx-gallery
# https://sphinx-gallery.github.io/
# which can be used by projects using plotly in their documentation.
# This module monkey-patches plotly.offline.plot, so we do not
Copy link
Contributor

Choose a reason for hiding this comment

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

We can now remove mention of monkey patching right?

@jonmmease
Copy link
Contributor

Thanks for making the renderer update @emmanuelle, this looks nice. I left a couple of comments inline.

But I could ask the sphinx-gallery team if they would accept a PR for extending their figure_rst function to other formats, they didn't want to maintain the scrapers but a figure_rst function with html support might be ok for them.

Sounds like it would be worth asking, as I'd expect this to be useful for other libraries as well. But it's not a lot of logic, so not a huge deal.

Oh and the tests failure seem to be due to something else than the tests added by this PR.

Yeah, this has been fixed on master so if you merge/rebase it should go away.

Regarding the example in doc/conf.py, I think I'd rather have this moved to a separate repository that has only the sphinx gallery example. If that sounds alright, could you move this to a new repo in the plotly org and remove it from this PR? Thanks!

@emmanuelle emmanuelle changed the title [WIP] scraper function for sphinx-gallery, in plotly.io Scraper function for sphinx-gallery, in plotly.io Jun 2, 2019
@emmanuelle
Copy link
Contributor Author

Hurray it's green! I think I addressed all your requests, I moved the sphinx files and gallery examples to plotly/plotly-sphinx-gallery.

@emmanuelle
Copy link
Contributor Author

@jonmmease no hurry but since you're always very reactive, I just wanted to make sure that this hasn't fallen off your radar :-).

@jonmmease
Copy link
Contributor

haha, thanks for the ping. I haven't forgotten 🙂 I should be able to take a look tomorrow. BTW, are you fine leaving this for v4 or would you like to get it out into the word before that?

@emmanuelle
Copy link
Contributor Author

v4 is fine, it should be in a month or so right? I'm used to slower development cycles so no pb :-).

jonmmease
jonmmease suggested changes Jun 5, 2019
View reviewed changes
Copy link
Contributor

@jonmmease jonmmease left a comment

Choose a reason for hiding this comment

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

This looks great @emmanuelle, a couple of small comments/requests below but otherwise good to go. Thanks!

plotly/tests/test_optional/test_jupyter/package.json Outdated Show resolved Hide resolved
plotly/io/_base_renderers.py Outdated Show resolved Hide resolved
plotly/io/_base_renderers.py Outdated Show resolved Hide resolved
plotly/io/_base_renderers.py Outdated Show resolved Hide resolved
@jonmmease jonmmease added the V4 label Jun 18, 2019
@emmanuelle
Copy link
Contributor Author

@jonmmease sorry to have let this idle for some time. I think the tests failure are not related to this PR. Shall I try to make a new PR to the v4_integration branch instead?

@jonmmease
Copy link
Contributor

Looks, good. Thanks!

@jonmmease jonmmease merged commit 3be34f7 into plotly:master Jul 3, 2019
@jonmmease
Copy link
Contributor

@emmanuelle, could you double check that your sphinx-gallery example works with the plotly==4.0.0a9 alpha release? Thanks!

@emmanuelle
Copy link
Contributor Author

Yes it works! Thank you very much @jonmmease :-)

cs48a pushed a commit to cs48a/plotly.py that referenced this pull request Jul 21, 2019
@emmanuelle @cs48a
Scraper function for sphinx-gallery, in plotly.io (plotly#1577)
9bacc9b 
* scraper function for sphinx-gallery, in plotly.io.  scraper function + skeleton of sphinx doc with minimal gallery (two examples)

* added psutil dep to tox.ini

* python 2 compatibility

* added orca dep to package.json for tests

* removed typo

* corrected bug

* python 2 compatibility

* added electron

* try to configure orca

* Moved test file so that it's tested by the orca build only.

* use custom renderer for sphinx-gallery

* removed sphinx files which are now in plotly-sphinx-gallery

+ changed name of renderer sphinx -> sphinx-gallery, and changed wording
of documentation

* Modified test with new renderer name

* try to fix py2 compatibility

* write_html instead of offline.plot

* change import order
@mkcor mkcor mentioned this pull request Jul 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonmmease jonmmease jonmmease approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@emmanuelle @jonmmease