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

WIP, DOC: new doc landing page; clean up doc structure #6694

Merged
merged 42 commits into from
Sep 2, 2019

Conversation

drammock
Copy link
Member

  • replaces documentation landing page (with the collapsible categories) with an overview page orienting users to the various types of documentation (tutorials, examples, glossary, API, legacy manual, and other stuff)
  • renames doc landing page in the top menu from "documentation" to "overview"
  • reorders top menu elements
  • adds/moves various pages to doc/overview subfolder (units, supported data formats, numerical precision, etc)
  • removes confusingly-named doc/tutorials folder (which contained reports and commands pages); these are now in overview (but could move elsewhere)
  • removes "contributing" page from installation TOC (makes it an orphan page, since it has a top-level menu link), but adds a crossref to it from the install pages.
  • expands FAQ entry for bug reports
  • reflows text in FAQ to fit 79-char column
  • better introductory paragraphs to glossary, examples, and tutorials
  • adds new TOC page for the legacy manual (there seem to be some near-duplicate pages in there, help wanted deciding what to keep)

@codecov
Copy link

codecov bot commented Aug 22, 2019

Codecov Report

Merging #6694 into master will decrease coverage by 1.33%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #6694      +/-   ##
==========================================
- Coverage   89.48%   88.14%   -1.34%     
==========================================
  Files         420      420              
  Lines       75474    75602     +128     
  Branches    12369    12384      +15     
==========================================
- Hits        67535    66639     -896     
- Misses       5134     6168    +1034     
+ Partials     2805     2795      -10

@larsoner
Copy link
Member

No idea why you're hitting this weird rst_prolog error. We don't mess with the rst_prolog as far as I know, and neither does sphinx-gallery, sphinx-bootstrap-theme, or numpydoc...

@larsoner
Copy link
Member

... but I can reproduce the error locally, which is weird.

@larsoner
Copy link
Member

larsoner commented Aug 22, 2019

Actually I had an idea. I did:

larsoner@bunk:~/python/mne-python$ git grep ":class:\`Annot"

And the only results are in whats_new.rst and glossary.rst.

It then occurred to me that the problem could be that the rendering order of the docs could have changed, which could lead effectively to a missing .. currentmodule:: mne from the top of some docs. We could add this to rst_prolog in conf.py, but I'm not sure if this would have adverse side effects. Also, it appears as though putting it at the top of doc/glossary.rst is enough, so I pushed a commit with it.

@larsoner
Copy link
Member

@drammock
Copy link
Member Author

@larsoner thanks for the currentmodule fix! If CIs are happy with my latest changes I think this is ready for review.

@drammock
Copy link
Member Author

Here's the new docs overview page: https://14869-1301584-gh.circle-artifacts.com/0/dev/overview/index.html

@larsoner
Copy link
Member

I like it! I don't like that there are so many miscellaneous links at the bottom, though, it actually draws the eye to that content. Ideas:

  • Put the Getting started with MNE web report content directly into a Notes docstring of the command line tool itself
  • All of these I would just make FAQ entries:
    • Floating-point precision
    • Supported data formats
    • Supported channel types
    • Internal representation (units)

Also, the naming "The web version of Matti Hämäläinen’s original MNE-C documentation" isn't 100% accurate anymore, because we do keep some things updated like the datasets list (and might have removed some content by turning it into tutorials at some point?). Maybe it's good enough to say "The updated web ..." or something

@larsoner
Copy link
Member

... and the Report probably does not need an image now that SG renders and embeds the HTML. Actually maybe we should just get rid of the command-line stuff altogether, it does not add much over what we see in the rendered example plus --help output. Adding a link from the command-line doc to the rendered example would be helpful, though.

@drammock
Copy link
Member Author

@larsoner I started migrating the Report examples in doc/overview/report.rst to the docstring of mne/commands/mne_report.py (command line examples) and the Notes section of mne/report.py:Report (python examples). Then I ran into #6708. I also am attempting to make the bash and python examples parallel, e.g.:

mne report --path ~/mne_data/MNE-sample-data/ --verbose

into corresponding Python code that yields the same output:

report = Report(verbose=True)
report.parse_folder('~/mne_data/MNE-sample-data/')
report.save()

Is that what you were envisioning?

@larsoner
Copy link
Member

I also am attempting to make the bash and python examples parallel, e.g.:

This is a noble goal but not strictly necessary. I would say do what you can and don't spend too much time on this. I would say that if people want configurability they should use the Report class in any case.

@drammock
Copy link
Member Author

remaining issues:

  • I expect 912ef24 to cause problems. It doesn't seem to break html_dev-noplot, but I removed some doctest skip and doctest ellipses comments so that probably broke something. I'm also not wild about having that much content in a Notes section of a docstring; seems like it really should be its own page(s) somewhere. Thoughts?
  • I can't seem to successfully crossref doc/generated/commands.rst with either a :ref: or a :doc: role. Which is weird because I was able to get it to show up in a :toctree: before by passing the ../generated/commands path.

@larsoner
Copy link
Member

I'm also not wild about having that much content in a Notes section of a docstring; seems like

Are you talking about mne report Notes section? If so I would just remove some content, and refer people to the Python example. From that and the mne report --help people can mostly figure out what's going on.

I can't seem to successfully crossref doc/generated/commands.rst with either a :ref: or a :doc: role.

You can dump the intersphinx inventory after doc build to see if it shows up under any name, e.g.:

python -msphinx.ext.intersphinx doc/_build/html/objects.inv > mne.txt

Links in that file definitely exist because we cross-ref them in whats_new.rst (e.g., gen_mne_browse_raw).

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

CIs are not happy, otherwise LGTM

mne/report.py Outdated
... captions='Left Auditory',
... section='evoked',
... replace=True) # doctest: +SKIP
>>> report.save('report.html', overwrite=True) # doctest: +SKIP
Copy link
Member

Choose a reason for hiding this comment

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

Actually maybe this can just be moved over to:

https://mne.tools/dev/auto_examples/visualization/plot_make_report.html#sphx-glr-auto-examples-visualization-plot-make-report-py

And moved to tutorials. Maybe in another PR.

@larsoner
Copy link
Member

Looks like you need another rebase

@drammock
Copy link
Member Author

here's the new docs landing page:
https://14965-1301584-gh.circle-artifacts.com/0/dev/overview/index.html

here's the new tutorial on mne.Report:
https://14965-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/misc/plot_report.html
It renders OK, but:

  • the covariance example seems to be returning a figure of "eigenvalue index vs noise" that gets rendered inline in the tutorial instead of added to the report HTML page.
  • the evoked plots look terrible (with and without whitening); am I doing something wrong there? They look fine when the figure is created separately using evoked.plot() and then added to the report as a custom figure.

@agramfort
Copy link
Member

agramfort commented Aug 28, 2019 via email

@drammock
Copy link
Member Author

Sorry @jona-sassenhagen, boldface screws up the cross-references: https://15021-1301584-gh.circle-artifacts.com/0/dev/overview/index.html

@lgtm-com
Copy link

lgtm-com bot commented Aug 29, 2019

This pull request introduces 1 alert when merging 04b96ac into 6d1aa1c - view on LGTM.com

new alerts:

  • 1 for Redundant assignment

img=img, id=global_id, div_klass='covariance',
img_klass='covariance', caption=caption, show=show,
image_format=image_format))
return '\n'.join(html)
Copy link
Member Author

Choose a reason for hiding this comment

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

This change captures both the covariance figure and the SVD figure, and iterate over them to add both to the report if they are both present. Previously, the SVD figure was automatically created, and ended up inline in the tutorial (rather than getting captured and put into the report HTML). Currently the choice of whether to show/not show the SVD plot is internal (e.g., user can't choose when instantiating mne.Report or when calling report.parse_folder). LMK if you think it should be exposed (and if so, where to expose it and under what name).

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 it should always be shown in the report

@drammock
Copy link
Member Author

Remaining questions:

  • should including the noise cov SVD plot in the report should be a user-exposed choice?
  • the "manual"
    • there are pages within the doc/manual folder that look like old/new versions of the same content (e.g., "the forward solution", "the minimum norm current estimates", morphing). Can someone explain why, and offer guidance as to which to keep?
    • If we're keeping both, should some be removed from the manual TOC (i.e., effectively hidden, only findable if you know the link in advance)?

@larsoner
Copy link
Member

should including the noise cov SVD plot in the report should be a user-exposed choice?

I think it's always useful to have. If it's there by default let's keep it. If it's not there, maybe we should just add it.

there are pages within the doc/manual folder that look like old/new versions of the same content

I'm not sure, it might be worth having a quick chat with @agramfort to long-term plan this

@drammock
Copy link
Member Author

should including the noise cov SVD plot in the report should be a user-exposed choice?

I think it's always useful to have. If it's there by default let's keep it. If it's not there, maybe we should just add it.

Well it's there by default now 🙂

there are pages within the doc/manual folder that look like old/new versions of the same content

I'm not sure, it might be worth having a quick chat with @agramfort to long-term plan this

@agramfort ping me on gitter to discuss? Or we could set up a 3-way skype/hangout if you think @larsoner should also weigh in.

@agramfort
Copy link
Member

agramfort commented Sep 1, 2019 via email

@agramfort agramfort merged commit d247e55 into mne-tools:master Sep 2, 2019
@agramfort
Copy link
Member

thx heaps @drammock !

@drammock drammock deleted the doc-restructure branch September 2, 2019 15:45
alexrockhill pushed a commit to alexrockhill/mne-python that referenced this pull request Oct 1, 2019
* better introductory paragraphs

* add overview files

* move files to overview folder

* cleanup overview landing page

* minor cleanup of install docs

* fix crossrefs

* update FAQ (esp. bug section; rest is mostly reflowing long lines)

* fix more crossrefs; move report and command_line into overview

* add output of gen_commands to toctree

* fix links/crossrefs/includes

* FIX: Refs

* fix: rST is not markdown

* fix: time != memory

* fix: minor tweaks (v0.12 is distant past now!)

* move precision, units, ch_types and data_formats to one page

* move includes to doc/_includes/ so they won't needlessly render as separate pages

* fix crossref

* move report examples into docstrings

* use standard heading "Page contents" in gen_commands

* move c-commands to MNE-C section; move intro note to top; remove learn_python from TOC and add to intro note

* remove title formatting

* fix crossref; smooth wording

* fix unescaped asterisk

* fix indentation

* fix pydocstyle

* fix crossref stupidity

* fix pydocstyle again

* fix travis?

* add warning to top of examples gallery

* fix long lines

* fix: swallow runtime warning about no subjects_dir set

* fix doctest?

* really fix doctest?

* give up on doctest; move to tutorial

* fix missing crossref

* fix: different filenames for each report (so the previews don't all render the same one)

* fix missing backtick

* add boldface per Jona's request

* capture and display the noise cov singular values plot in reports

* use baselined evoked file

* Revert "add boldface per Jona's request"

This reverts commit e03bfcd.

* fix LGTM [skip travis][skip azp][circle skip]
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.

4 participants