Closed larsoner closed 7 years ago
@Eric89GXL can you share a rendered site to see?
@agramfort finally here:
https://ci.appveyor.com/api/buildjobs/o8v5r82fmmgbjhw0/artifacts/doc%2F_build%2Fhtml.zip
looks great
Thanks for taking the initiative here, Eric. The new "modern" css looks mostly good. Here are some thoughts:
The box on the index page with the logo/name/tagline (I think web devs call this a "hero" element?) feels too large for its relatively uninteresting visual content.
The bootswatch theme in general is clean and nice, but the lack of any color on pages other than the homepage feels very bland.
By moving the first sentence of the intro description into the tagline, we start with "PySurfer is primarily intended for use with Freesurfer, " which does not grab my attention! This paragraph needs some improvement.
Jumbotron :)
With these aesthetic choices we risk getting into a back-and-forth bikeshed-type argument because there is no clear right answer. So I'll take a stab at addressing these comments, but I'd like to stop at whatever is the minimal acceptable documentation improvement, and iterate over subsequent PRs (probably not by me) to tweak such aesthetic / layout / wordings further. Okay?
I don't like how we've lost the links to other parts of the site from the main text on the homepage.
I disagree on this point. They are redundant with the links at the top. If we put them on the main page, they should really have identical text / naming as the top links. So unless the links at the top need some additional explanation that you could put to the right of such main-body links (and I don't think they do), I don't see a reason to also have them on the main page.
I just noticed that our install page still says "does not work on Python 3", which is no longer True. Also do we have a hard dependency on IPython still?
Technically Python 3 is possible if you use non-standard Anaconda repos. I won't add that for now (can be a separate PR because the wording might be tricky), but I'll remove the bit about IPython.
"Documentation" in the header should probably be "Tutorial" or something more specific, since everything is really "documentation". Although the stuff in there isn't a very good tutorial.... Also "Examples" should maybe be "Gallery".
I'll change these.
Images in the example gallery are not working (at least in the static Appveyor download).
Yes AppVeyor only does the "noplot" build.
So I'll take a stab at addressing these comments, but I'd like to stop at whatever is the minimal acceptable documentation improvement, and iterate over subsequent PRs (probably not by me) to tweak such aesthetic / layout / wordings further. Okay?
Well ... sure ... but I don't see the point of merging a PR that is aesthetically/textually a regression from the current docs. Unless I'm missing something, adding automatically generated API docs and moving the example gallery to sphinx-gallery is orthogonal to using sphinx-bootswatch and altering the homepage text. So I think it's fine for a PR about the former not to get bogged down in a discussion of the latter, but maybe they should be separate PRs/discussions.
I disagree on this point. They are redundant with the links at the top. If we put them on the main page, they should really have identical text / naming as the top links. So unless the links at the top need some additional explanation that you could put to the right of such main-body links (and I don't think they do), I don't see a reason to also have them on the main page.
They are not redundant because they serve different purposes. The links at the top are navigational elements for people who know what they are looking for. The main text on the homepage should tell new visitors what there is to look for. I agree that the latter goal is better served by a bit of narration about the different elements of the website than just a list of links and that nomenclature should be standardized. You can look at the seaborn homepage to get an idea of what my preference would be.
I don't see the point of merging a PR that is aesthetically/textually a regression from the current docs.
I agree, but I didn't realize you considered these (and I don't consider these) aesthetic changes as regressions. If you think of them the changes way, they should be separated so I'll revert.
If we end up sticking with the sphinx_bootstrap/bootswatch themes, one thing I hate about the CSS is the giant text in the API parameter descriptions; in the seaborn docs I make it smaller.
I agree, but I didn't realize you considered these (and I don't consider these) aesthetic changes as regressions. If you think of them the changes way, they should be separated so I'll revert.
Perhaps its just my familiarity (and authorship) bias, but the new site with the bland color palette and reduce text feels very unfinished and unwelcoming. I think it can be made better than the original docs with some judicious changes, even simple things like including a small version of the logo in the header. I recognize that bikeshedding about aesthetics can be annoying, but you are kind of inviting it when you propose a whole-sale restyling of the docs...
I recognize that bikeshedding about aesthetics can be annoying, but you are kind of inviting it when you propose a whole-sale restyling of the docs...
Yeah this is true. I wrongly / naively assumed that folks would agree with me main thrust of the changes. I have now reverted them in any case so we can deal with them separately, so this should now hopefully just be functional stuff switching to sphinx-gallery etc. now.
New build:
https://ci.appveyor.com/api/buildjobs/g7a95ykjrgaqunaj/artifacts/doc%2F_build%2Fhtml.zip
Travis is just waiting for OSX to build, but should come back green.
CIs are happy, ready for review/merge from my end.
FWIW talking to @agramfort he didn't like the modern look either!
I tried building the docs (make html_stable
) but had some problems. A few examples complained about not finding the fsaverage BA labels -- I think this is because Freesurfer 6.0 added "exvivo" to their names. The second issue is crashing out of quite a few examples because it's looking for the SUBJECTS_DIR in /Applications/freesurfer/subjects
. But that's not where my SUBJECTS_DIR is, which the $SUBJECTS_DIR variable in my terminal confirms. Is there some reason that the changes make pysurfer think it's running on a mac? git grep Applications
is empty...
Here are the exceptions:
Exception occurred:
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/sphinx_gallery/gen_gallery.py", line 280, in sumarize_failing_examples
"\n" + "-" * 79)
ValueError: Here is a summary of the problems encountered when running the examples
Unexpected failing examples:
/home/mwaskom/code/pysurfer/examples/plot_resting_correlations.py failed leaving traceback:
Traceback (most recent call last):
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/sphinx_gallery/gen_rst.py", line 472, in execute_code_block
exec(code_block, example_globals)
File "<string>", line 8, in <module>
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/surfer/viz.py", line 416, in __init__
subjects_dir = _get_subjects_dir(subjects_dir=subjects_dir)
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/surfer/utils.py", line 659, in _get_subjects_dir
% subjects_dir)
ValueError: The subjects directory /Applications/freesurfer/subjects does not exist.
/home/mwaskom/code/pysurfer/examples/plot_transparent_brain.py failed leaving traceback:
Traceback (most recent call last):
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/sphinx_gallery/gen_rst.py", line 472, in execute_code_block
exec(code_block, example_globals)
File "<string>", line 16, in <module>
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/surfer/viz.py", line 416, in __init__
subjects_dir = _get_subjects_dir(subjects_dir=subjects_dir)
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/surfer/utils.py", line 659, in _get_subjects_dir
% subjects_dir)
ValueError: The subjects directory /Applications/freesurfer/subjects does not exist.
/home/mwaskom/code/pysurfer/examples/plot_label.py failed leaving traceback:
Traceback (most recent call last):
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/sphinx_gallery/gen_rst.py", line 472, in execute_code_block
exec(code_block, example_globals)
File "<string>", line 13, in <module>
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/surfer/viz.py", line 1289, in add_label
% filepath)
ValueError: Label file /usr/local/freesurfer/current/subjects/fsaverage/label/lh.BA1.label does not exist
/home/mwaskom/code/pysurfer/examples/plot_topographic_contours.py failed leaving traceback:
Traceback (most recent call last):
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/sphinx_gallery/gen_rst.py", line 472, in execute_code_block
exec(code_block, example_globals)
File "<string>", line 13, in <module>
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/surfer/viz.py", line 416, in __init__
subjects_dir = _get_subjects_dir(subjects_dir=subjects_dir)
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/surfer/utils.py", line 659, in _get_subjects_dir
% subjects_dir)
ValueError: The subjects directory /Applications/freesurfer/subjects does not exist.
/home/mwaskom/code/pysurfer/examples/plot_probabilistic_label.py failed leaving traceback:
Traceback (most recent call last):
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/sphinx_gallery/gen_rst.py", line 472, in execute_code_block
exec(code_block, example_globals)
File "<string>", line 10, in <module>
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/surfer/viz.py", line 416, in __init__
subjects_dir = _get_subjects_dir(subjects_dir=subjects_dir)
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/surfer/utils.py", line 659, in _get_subjects_dir
% subjects_dir)
ValueError: The subjects directory /Applications/freesurfer/subjects does not exist.
-------------------------------------------------------------------------------
The full traceback has been saved in /tmp/sphinx-err-oK1cAm.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
Makefile:42: recipe for target 'html_stable' failed
make: *** [html_stable] Error 1
can you revert the doc/_static/banner.png changes?
I can revert the banner but it seemed useful to leave it transparent ?
initial banner looks nicer. I seemed to see visual artifacts on circles.
Ahh I must have done a bad job of making it transparent. I'll revert it.
Is there some reason that the changes make pysurfer think it's running on a mac? git grep Applications is empty...
Shouldn't be, but I can check. Are you on Linux or Windows?
Comments addressed. @mwaskom build problems should be fixed, feel free to try again.
I still get one error on Freesurfer 6.0
Exception occurred:
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/sphinx_gallery/gen_gallery.py", line 280, in sumarize_failing_examples
"\n" + "-" * 79)
ValueError: Here is a summary of the problems encountered when running the examples
Unexpected failing examples:
/home/mwaskom/code/pysurfer/examples/plot_label.py failed leaving traceback:
Traceback (most recent call last):
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/sphinx_gallery/gen_rst.py", line 472, in execute_code_block
exec(code_block, example_globals)
File "<string>", line 40, in <module>
File "/home/mwaskom/anaconda2/lib/python2.7/site-packages/surfer/viz.py", line 1289, in add_label
% filepath)
ValueError: Label file /usr/local/freesurfer/current/subjects/fsaverage/label/lh.entorhinal.label does not exist
The new label is ?h.entorhinal_exvivo.label
.
In the header there is a link to the "modules" page, which isn't that useful for PySurfer. Could that be changed to link directly to the python_reference.html
page?
I bet this is not related to the doc changes, but I just noticed in the gallery that the negative side of Overlays has a backwards colorbar. @christianbrodbeck is this related to your recent PR do you think?
I did modify some overlay code, I'll check, which example?
It's apparent in the gallery in the plot_fmri_volume
example which shows the negative side of the overlay colormap, and you can see it partway through running the plot_resting_correlations
example.
Yes I can confirm, I'll work on a fix
edit: #196
In the header there is a link to the "modules" page, which isn't that useful for PySurfer. Could that be changed to link directly to the python_reference.html page?
Maybe, but I don't have time to look,let's do it in another PR. For now I've reverted the change that added those links.
I also fixed the label / build problem.
Maybe, but I don't have time to look,let's do it in another PR. For now I've reverted the change that added those links.
Sounds good — our API is a little weird in that pretty much everything you'd want to know is handled by methods on Brain
(sigh) so it's worth thinking about how to best organize the API docs so people can get that information with minimal clicking. Totally fine with punting on those details, though.
CIs are happy, let me know if there's anything else needed for this one.
Looks good for now, thanks!
This PR:
These were necessary to get things looking good on OSX.