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

[MRG+1] Backreferences template from sphinx-gallery #592

Merged
merged 12 commits into from
Nov 28, 2015

Conversation

Titan-C
Copy link
Contributor

@Titan-C Titan-C commented May 20, 2015

Here I update to sphinx-gallery with the backreferences solution.
So now it is possible to use an rst template to document classes and functions including at the end the link to examples using them.
A question of design. Since now this documentation generation is done with default sphinx extention autodoc. It becomes a dependency to have it in the included extensions in the conf.py file.(Not an issue since it was already used by nilearn). But because it is an automatic process all automatic documented classes will try to have the backreferences to example, which might not exist. This issues a warning to during make. To workaround this warning scikit-learn and now in this PR, there is an extra action in conf.py that touches non-existing backreference example files so that one does not get the include warning. Question:

  • Is this the good way for the user to proceed?(I'll write documentation for this procedure in sphinx-gallery)
  • Alternative: have this action as default in sphinx-gallery and require user to always have autodoc extension always loaded?

Massive renaming in files because:
Since the new naming convention for sphinx-gallery image prefix sphx-glr image references are updated.
Since naming convention for example references is to include the target dir, this references are also updated.

@lesteve
Copy link
Contributor

lesteve commented Jun 2, 2015

To workaround this warning scikit-learn and now in this PR, there is an extra action in conf.py that touches non-existing backreference example files so that one does not get the include warning

Just wondering wheter there is a way to have some logic in the jinja2 template to say that if the .examples file exists include it otherwise don't bother. Seems slightly better than creating empty .examples files. By the way I am not sure about the ordering of gen_gallery versus the 'autodoc-process-docstring' step. Imagine you start from a clean state, are all the .examples file created empty during the 'autodoc-process-docstring' and then gen_gallery comes along and generates non-empty .examples file or is it the other way around? I think what I am trying to say is that it does seem a bit fragile.

Since naming convention for example references is to include the target dir, this references are also updated.

That makes the naming a bit too redundant: example_auto_examples_... Just wondering whether there is an easy but generic way to remove auto_examples from the reference name.

Also you need to rebase on master and fix the merge conflicts.

@Titan-C
Copy link
Contributor Author

Titan-C commented Jun 2, 2015

. By the way I am not sure about the ordering of gen_gallery versus the 'autodoc-process-docstring' step.

Sphinx gallery does its job first creating the .example files. 'autodoc-process-docstring' is executed afterwards everytime it finds the include directive to the example file

That makes the naming a bit too redundant: example_auto_examples_

Yes, I agree on the redundancy. At some point I tested to rename it to sphx_glr_auto_example_ or to remove completely the example_ prefix, but I could not agree with myself on which to prefer so I left it that way. I support the fact to have the full target name on the reference starting from the sphinx source folder, so to our convention the gallery sits in auto_examples_. That way is was easier to link to the images and references. To put an additional prefix to it might not be necessary anymore, example_ was used because all paths where used taken the inside of the gallery folder as reference, so having the prefix became valuable. As a generic option I would rather remove example_ prefix than the first folder auto_example

@Titan-C
Copy link
Contributor Author

Titan-C commented Jun 2, 2015

rebase done

1 similar comment
@Titan-C
Copy link
Contributor Author

Titan-C commented Jun 2, 2015

rebase done

@lesteve
Copy link
Contributor

lesteve commented Jun 2, 2015

As a generic option I would rather remove example_ prefix than the first folder auto_example

I am not sure, it feels like the reference name should look more like the example python file than the generated rst in auto_examples. Also it'd be good to have some consistency between the generated image name and the reference name. At the moment the two of them have grown further apart.

Sphinx gallery does its job first creating the .example files. 'autodoc-process-docstring' is executed afterwards everytime it finds the include directive to the example file

Surely this can't be true, 'autodoc-process-docstring' doesn't know anything about example files which are sphinxgallery specific. The only way this can work if the autodoc-process-string creates all example files as empty and then sphinxgallery comes along, parses the example scripts and overrides the .examples files. Just curious, is the ordering guaranteed ? Does it depend on the ordering of extensions in conf.py ?

More importantly, it seems that you haven't included the backreferences.py module in this PR (i.e. sphinxext/sphinxgallery/backreferences.py) is that on purpose?

@Titan-C
Copy link
Contributor Author

Titan-C commented Jun 2, 2015

I am not sure, it feels like the reference name should look more like the example python file than the generated rst in auto_examples. Also it'd be good to have some consistency between the generated image name and the reference name. At the moment the two of them have grown further apart.

For the consistency with image names I wanted to put it as prefix sphx_glr, then the path to the python example. That way is more consistent.

Surely this can't be true, 'autodoc-process-docstring' doesn't know anything about example files which are sphinxgallery specific.

Just curious, is the ordering guaranteed ? Does it depend on the ordering of extensions in conf.py ?

Yes the ordering is guaranteed. As each one is assigned a different event in the sphinx build. It does not depend in the ordering of the extensions in conf.py but to which build step it was assigned. Sphinx-gallery happens when the builder is started(outputs rst files, image files, .example files), but the autodoc file touch is when rst files are parsed.
So autodoc is completely independent of sphinx-gallery, it only tries to include a matching example file to every module it has to document. If it can't find the file a warning is issued, and in our case the missing file is touched and so no warning issued.

More importantly, it seems that you haven't included the backreferences.py module in this PR (i.e. sphinxext/sphinxgallery/backreferences.py) is that on purpose?

This is a glitch in my update script as it does not git add new files. I'll fix it

@lesteve lesteve changed the title Backreferes template from sphinx-gallery Backreferences template from sphinx-gallery Jun 3, 2015
@lesteve
Copy link
Contributor

lesteve commented Jun 29, 2015

This needs a rebase on master actually.

@GaelVaroquaux
Copy link
Member

What's the status on this? Should I try to merge this? I would really like to have sphinx-gallery in nilearn, and then upgrade it to have the notebook-style examples.

@@ -1,8 +1,9 @@
"""Sphinx Gallery
"""
import os
__version__ = '0.0.9-dev'
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we should stick with a released version. @Titan-C any reason why this is not 0.0.11?

@Titan-C Titan-C force-pushed the backref_template branch 2 times, most recently from 8efb102 to 83ba236 Compare November 24, 2015 17:04
@Titan-C
Copy link
Contributor Author

Titan-C commented Nov 24, 2015

This one is rebased on master and has the latest release of Sphinx-Gallery.
Some examples already have the notebook style. All seem to be running now.

There is one link that is not working and I hesitate to change.

https://github.com/nilearn/nilearn/blame/master/doc/connectivity/functional_connectomes.rst#L138-L142
file examples/manipulating_visualizing/plot_probabilistic_atlas.py does not exist. On same folder there is one plot_prob_atlas.py, but since there the following figure does not match the file either nor can I find mention of the function stated before the link I'm not sure.

you can preview the website in

http://scikit-learn.byethost7.com/nilearn/

@Titan-C
Copy link
Contributor Author

Titan-C commented Nov 24, 2015

Side question. How does the circleci work. Can I see the sphinx output there? the rendered documentation?

@KamalakerDadi
Copy link
Contributor

@Titan-C Thanks for reporting.

examples/manipulating_visualizing/plot_probabilistic_atlas.py does not exist since the name plot_probabilistic_atlas is renamed as plot_overlay to avoid conflicting with plot_prob_atlas. Both plot_overlay and plot_prob_atlas exists in Nilearn.

Could you please change this
see the :ref:corresponding example <example_manipulating_visualizing_plot_probabilistic_atlas.py>

to this
see the :ref:corresponding example <example_manipulating_visualizing_plot_overlay.py>

Then I think it will work. Please let me know. If it doesn't ?

@GaelVaroquaux
Copy link
Member

@Titan-C : the circle-ci web interface does not give you links to artifacts unless you are administrator :(.

But here is the link to the latest build of your PR:
https://circle-artifacts.com/gh/nilearn/nilearn/733/artifacts/0/home/ubuntu/nilearn/doc/_build/html/auto_examples/index.html

@@ -292,7 +292,18 @@
'sklearn': 'http://scikit-learn.org/stable'}
}


def touch_example_backreferences(app, what, name, obj, options, lines):
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker to merge this PR, but should this be in sphinx_gallery itself? It seems that it's something that people can easily get wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about it. In general you don't need this. It is only to remove some warnings of sphinx as it runs autosummary.

Copy link
Member

Choose a reason for hiding this comment

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

+def touch_example_backreferences(app, what, name, obj, options, lines):

I was also thinking about it. In general you don't need this. It is only to
remove some warnings of sphinx as it runs autosummary.

If it's easy to add, I think you should (though not in this PR)

@GaelVaroquaux GaelVaroquaux changed the title Backreferences template from sphinx-gallery [MRG+1] Backreferences template from sphinx-gallery Nov 25, 2015
@GaelVaroquaux
Copy link
Member

+1 for merge. This is simply great. Thank you very @Titan-C

Does anyone wants to have a look, or should I merge?

@KamalakerDadi
Copy link
Contributor

Does anyone wants to have a look, or should I merge?

I want to have a look. I will give my call by end of day.

@Titan-C
Copy link
Contributor Author

Titan-C commented Nov 25, 2015

@Titan-C : the circle-ci web interface does not give you links to artifacts unless you are administrator :(.

But here is the link to the latest build of your PR:
https://circle-artifacts.com/gh/nilearn/nilearn/733/artifacts/0/home/ubuntu/nilearn/doc/_build/html/auto_examples/index.html

That's cool. Then I would need to manually upload the builds to my test website

@lesteve
Copy link
Contributor

lesteve commented Nov 26, 2015

@Titan-C just curious, is there an example that renders as notebook in https://circle-artifacts.com/gh/nilearn/nilearn/733/artifacts/0/home/ubuntu/nilearn/doc/_build/html/auto_examples/index.html. I haven't found any yet.

@GaelVaroquaux
Copy link
Member

@lesteve
https://circle-artifacts.com/gh/nilearn/nilearn/733/artifacts/0/home/ubuntu/nilearn/doc/_build/html/auto_examples/decoding/plot_haxby_different_estimators.html#sphx-glr-auto-examples-decoding-plot-haxby-different-estimators-py

And actually we can see that from a layout / styling perspective, the code line and the output of the script appear as visually similar, which can be confusing for the users. We will need to change this.

@lesteve
Copy link
Contributor

lesteve commented Nov 26, 2015

Thanks a lot.

And actually we can see that from a layout / styling perspective, the code line and the output of the script appear as visually similar,

There is an issue about that already sphinx-gallery/sphinx-gallery#44

@GaelVaroquaux
Copy link
Member

GaelVaroquaux commented Nov 26, 2015 via email

@GaelVaroquaux
Copy link
Member

There is an issue about that already sphinx-gallery/sphinx-gallery#44

I pushed a quick fix:
sphinx-gallery/sphinx-gallery#72

we can backport in nilearn.

@GaelVaroquaux
Copy link
Member

Merging this guy, and backporting the latest sphinx-gallery, to have the new style.

GaelVaroquaux added a commit that referenced this pull request Nov 28, 2015
[MRG+1] Backreferences template from sphinx-gallery
@GaelVaroquaux GaelVaroquaux merged commit 424c905 into nilearn:master Nov 28, 2015

easy_install -Zeab tmp sphinx-gallery

cp -vru tmp/sphinx-gallery/sphinxgallery/ .
Copy link
Member

Choose a reason for hiding this comment

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

I just realized: why was this file deleted?

@Titan-C Titan-C deleted the backref_template branch August 3, 2016 23:50
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

4 participants