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

refactor docs to use docstrings and have a gallery #1116

Merged
merged 17 commits into from
Jun 2, 2018

Conversation

bjarthur
Copy link
Member

@bjarthur bjarthur commented Mar 17, 2018

so two things that'd be nice to have are:

  1. a matplotlib style gallery, where example plots are all collected in one place along with the exact code to reproduce them.

  2. julia style docstrings so that documentation can be accessed on the REPL. docs embedded in the code are also less likely to get out of date.

towards these ends, i've started down the path of refactoring the documentation. it's a big change though, so this PR currently only contains 10% or so of the changes needed, as i want to get feedback before proceeding.

would be great if folks could checkout this PR, do a julia docs/make.jl, then open docs/build/index.html and have a look around. also do a ?Geom, ?Geom.bar, etc. on the REPL. i've only made galleries for, and altered the libraries of, two Geoms and one Guide. the rest will follow suit if you guys like this new format. let me know! thanks.

@codecov-io
Copy link

codecov-io commented Mar 17, 2018

Codecov Report

Merging #1116 into master will decrease coverage by 0.21%.
The diff coverage is 73.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1116      +/-   ##
==========================================
- Coverage   87.28%   87.07%   -0.22%     
==========================================
  Files          33       33              
  Lines        3973     3984      +11     
==========================================
+ Hits         3468     3469       +1     
- Misses        505      515      +10
Impacted Files Coverage Δ
src/geom/hvabline.jl 94.11% <ø> (ø) ⬆️
src/coord.jl 83.5% <ø> (ø) ⬆️
src/geom/hexbin.jl 95.23% <ø> (ø) ⬆️
src/geom/line.jl 90.21% <ø> (ø) ⬆️
src/guide.jl 85.92% <ø> (ø) ⬆️
src/geom/point.jl 100% <ø> (ø) ⬆️
src/geom/boxplot.jl 100% <ø> (ø) ⬆️
src/geom/label.jl 99.09% <ø> (ø) ⬆️
src/misc.jl 55.61% <ø> (ø) ⬆️
src/geom/polygon.jl 66.66% <ø> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa57f8f...fec7e18. Read the comment docs.

@tlnagy
Copy link
Member

tlnagy commented Mar 21, 2018

Doesn't build for me?

$ julia docs/make.jl
Documenter: setting up build directory.
Documenter: expanding markdown templates.
ERROR: LoadError: MethodError: no method matching discretize_make_pda(::CategoricalArrays.CategoricalArray{String,1,UInt8,String,CategoricalArrays.CategoricalString{UInt8},Union{}}, ::Array{CategoricalArrays.CategoricalString{UInt8},1})
Closest candidates are:
  discretize_make_pda(!Matched::Array{T,1} where T, ::Any) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/scale.jl:263
  discretize_make_pda(!Matched::DataArrays.DataArray, ::Any) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/scale.jl:271
  discretize_make_pda(!Matched::Range, ::Any) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/scale.jl:279
  ...
Stacktrace:
 [1] discretize(::CategoricalArrays.CategoricalArray{String,1,UInt8,String,CategoricalArrays.CategoricalString{UInt8},Union{}}, ::Void, ::Void, ::Bool) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/scale.jl:301
 [2] discretize(::CategoricalArrays.CategoricalArray{String,1,UInt8,String,CategoricalArrays.CategoricalString{UInt8},Union{}}, ::Void, ::Void) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/scale.jl:295
 [3] apply_scale(::Gadfly.Scale.DiscreteScale, ::Array{Gadfly.Aesthetics,1}, ::Gadfly.Data, ::Vararg{Gadfly.Data,N} where N) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/scale.jl:366
 [4] apply_scales(::IterTools.Distinct{Base.ValueIterator{Dict{Symbol,Gadfly.ScaleElement}},Gadfly.ScaleElement}, ::Array{Gadfly.Aesthetics,1}, ::Gadfly.Data, ::Vararg{Gadfly.Data,N} where N) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/scale.jl:33
 [5] apply_scales(::IterTools.Distinct{Base.ValueIterator{Dict{Symbol,Gadfly.ScaleElement}},Gadfly.ScaleElement}, ::Gadfly.Data) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/scale.jl:52
 [6] render_prepare(::Gadfly.Plot) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/Gadfly.jl:674
 [7] render(::Gadfly.Plot) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/Gadfly.jl:752
 [8] show at /Users/tamasnagy/.julia/v0.6/Gadfly/src/Gadfly.jl:956 [inlined]
 [9] verbose_show(::Base.AbstractIOBuffer{Array{UInt8,1}}, ::MIME{Symbol("image/svg+xml")}, ::Gadfly.Plot) at ./multimedia.jl:42
 [10] #sprint#228(::Void, ::Function, ::Int64, ::Function, ::MIME{Symbol("image/svg+xml")}, ::Vararg{Any,N} where N) at ./strings/io.jl:66
 [11] stringmime(::MIME{Symbol("image/svg+xml")}, ::Gadfly.Plot) at ./multimedia.jl:83
 [12] runner(::Type{Documenter.Expanders.ExampleBlocks}, ::Base.Markdown.Code, ::Documenter.Documents.Page, ::Documenter.Documents.Document) at /Users/tamasnagy/.julia/v0.6/Documenter/src/Expanders.jl:477
 [13] dispatch(::Type{Documenter.Expanders.ExpanderPipeline}, ::Base.Markdown.Code, ::Vararg{Any,N} where N) at /Users/tamasnagy/.julia/v0.6/Documenter/src/Selectors.jl:168
 [14] expand(::Documenter.Documents.Document) at /Users/tamasnagy/.julia/v0.6/Documenter/src/Expanders.jl:30
 [15] runner(::Type{Documenter.Builder.ExpandTemplates}, ::Documenter.Documents.Document) at /Users/tamasnagy/.julia/v0.6/Documenter/src/Builder.jl:178
 [16] dispatch(::Type{Documenter.Builder.DocumentPipeline}, ::Documenter.Documents.Document, ::Vararg{Documenter.Documents.Document,N} where N) at /Users/tamasnagy/.julia/v0.6/Documenter/src/Selectors.jl:168
 [17] cd(::Documenter.##2#3{Documenter.Documents.Document}, ::String) at ./file.jl:70
 [18] #makedocs#1(::Bool, ::Array{Any,1}, ::Function) at /Users/tamasnagy/.julia/v0.6/Documenter/src/Documenter.jl:203
 [19] (::Documenter.#kw##makedocs)(::Array{Any,1}, ::Documenter.#makedocs) at ./<missing>:0
 [20] include_from_node1(::String) at ./loading.jl:576
 [21] include(::String) at ./sysimg.jl:14
 [22] process_options(::Base.JLOptions) at ./client.jl:305
 [23] _start() at ./client.jl:371
while loading /Users/tamasnagy/.julia/v0.6/Gadfly/docs/make.jl, in expression starting on line 5

@bjarthur
Copy link
Member Author

bjarthur commented Apr 1, 2018

not sure. maybe try again now that we've merged a bunch of PRs? thanks.

@bjarthur bjarthur force-pushed the bja/docs branch 2 times, most recently from 7d4a1dc to 123c1bf Compare April 2, 2018 14:20
@bjarthur
Copy link
Member Author

bjarthur commented Apr 3, 2018

@tlnagy did this error cause the entire build to fail, or just one example? if the latter, just let it finish and then take a look at the as-built docs. i'd like to proceed with finishing this PR if you like the direction it's headed.

@Mattriks
Copy link
Member

Mattriks commented Apr 3, 2018

I looked at this (it built for me). In the gallery, I would like each code block do more than one plot (with hstack). E.g. for each Geom I would like 2-3 single plots to be side by side (if a Geom has >3 plots, than using 2 code blocks = 2 rows of plots is ok). I think the plot size is right, but can the font size of the code blocks be reduced?, and hide the using Gadfly, RDatasets (but add at the top of the Gallery that the Gallery uses RDatasets). Also, the set_default_plot_size line is visible for me (I can see the # hide statement). With these changes, the Gallery should look awesome!

@bjarthur
Copy link
Member Author

bjarthur commented Apr 3, 2018

great idea about combing plots into subplots. i've made the changed and pushed.

i'm not keen though on separating out the common code elements and putting them at the top. i really want users to be able to make a single cut & paste to reproduce.

Guide.ylabel("Freq"));

rename!(D, :x1 => :Frequency)
palette = ["blue","brown","green","tan"] # Is there a hazel color?
Copy link
Member

Choose a reason for hiding this comment

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

Leftover comment? This is unnecessary

Copy link
Member

@Mattriks Mattriks Apr 4, 2018

Choose a reason for hiding this comment

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

Not leftover, it's a humorous comment! Hazel is a color (think eyes/hazelnut), and the plot is about eye color, so one should be able to use "hazel"!
I'm attempting to encourage the use of humorous code comments in the docs 😃

Copy link
Member

Choose a reason for hiding this comment

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

Ah. I get it.

@tlnagy
Copy link
Member

tlnagy commented Apr 3, 2018

did this error cause the entire build to fail, or just one example?

The whole build, but I pulled the latest changes and now I can't reproduce and everything is building 👍

i'm not keen though on separating out the common code elements and putting them at the top. i really want users to be able to make a single cut & paste to reproduce.

I agree with @bjarthur here.

would be great if folks could checkout this PR, do a julia docs/make.jl, then open docs/build/index.html and have a look around.

Overall, I really like this! I think one thing that's missing is code examples in the docstrings for the Geoms/Guides/etc (e.g. https://juliadocs.github.io/Documenter.jl/latest/lib/public/#Documenter.generate) so a user can see a concrete example of how a function can be used after running ?function.

@Mattriks
Copy link
Member

Mattriks commented Apr 4, 2018

Great! I particularly like the 3 plots for Guide.xlabel and Guide.ylabel. For the eye color plot in Geom.bar I think hstack(p1, p2a, p2b) looks better than the double hstack. The palette needs to be changed to [brown, blue, tan, green] so that it matches the eye color labels.

I would also like smaller font size for the code blocks, and/or where possible one-line plot statements e.g. for Geom.point p4 the Geom.point could be dropped from the plot statement, so it fits on one line.

Also about the common code elements, if you were a user doing copy and paste, would you copy and paste the using Gadfly, RDatasets line each time?

@bjarthur bjarthur force-pushed the bja/docs branch 6 times, most recently from d24a88d to 442135b Compare April 10, 2018 12:06
@bjarthur bjarthur force-pushed the bja/docs branch 3 times, most recently from f352135 to a1db3e0 Compare April 18, 2018 21:56
@bjarthur bjarthur mentioned this pull request Apr 27, 2018
@bjarthur bjarthur force-pushed the bja/docs branch 2 times, most recently from da38bd8 to 5d69220 Compare April 30, 2018 12:02
@bjarthur bjarthur mentioned this pull request May 1, 2018
6 tasks
@bjarthur bjarthur force-pushed the bja/docs branch 2 times, most recently from c8ff368 to feee5a6 Compare May 3, 2018 12:06
@bjarthur
Copy link
Member Author

bjarthur commented May 6, 2018

still working on this, and have come across a few things for which i'll submit issues for later. just as a reminder for myself:

  1. Stat.violin is very similar to Stat.density. Geom.violin might be able to be refactored to us it instead.

  2. what's the difference between Geom.histogram(density=true) and Geom.density()? should we deprecate the former?

  3. order in Geom.line, Geom.polygon, and Geom.violin would more descriptively be called zorder. is it even necessary though given that z-order is set by the input argument order in plot?

  4. why would one ever want to set preserve_order to false in Geom.contour?

  5. should think about refactoring struct RectangularBinGeometry to use a default statistic of Identity and renaming it RectangularGeometry. Geom.rectbin would then just use it as is, and Geom.rectbin would use Stat.rectbin. as it stands now, Geom.rect overrides the default Stat.rectbin with Stat.Identity, which is backwards from all other geometries. should also consider renaming rectbin to tile is discussed previously.

  6. Stat.binmean doesn't appear to be used internally. remove it?

@bjarthur
Copy link
Member Author

i'm confused... the number of examples should be the same. all i did was take the english text in docs/src/libs/geom and put it into a docstring in src/geom so that one could use the REPL's help system (the question mark command). what remains are the examples, which i simply moved into docs/src/gallery and reformatted a bit.

@tlnagy
Copy link
Member

tlnagy commented May 30, 2018

i'm confused... the number of examples should be the same.

My bad, Github was autocollapsing the geometries gallery so my searching for examples wasn't coming up.

In general, I'm a big fan of moving all the text to the docstrings and I think we should go ahead and merge this PR. I do however have some hesitation with how everything is laid out in the big monolithic files (e.g. docs/src/gallery/geometries.md). I think in a future PR we should explore transitioning the gallery pages to strictly a gallery (i.e. only images, no code). Then clicking on the image or the text could open a dedicated page that has a lot more detail on the function and lots more examples. I'm a big fan of how seaborn handles this: https://seaborn.pydata.org/examples/index.html but I would prefer to have the names visible without hovering.

@Mattriks
Copy link
Member

Mattriks commented May 31, 2018

Here are some suggestions. In the Gallery:

  • Geom.bar (2nd code block) do hstack(p1, p2a, p2b) rather than hstack(p1, hstack(p2a, p2b))
  • Scale.color_continuous (2nd code block) do
x = repeat(collect(1:10)-0.5, inner=[10])
y = repeat(collect(1:10)-0.5, outer=[10])

so the grid fills the whole plot.

  • Scale.color_discrete_manual (1st example) for me, this plot contains no points, but has a color key - do you see the points?
  • Scale.{x,y}_discrete: Scale.x_discrete(levels=df.colindex.names) can be changed to Scale.x_discrete(levels=names(df))

@Mattriks
Copy link
Member

And in the Library:

  • Gadfly page: the code examples for Compose.hstack and vstack start with 3 ticks but don't end with 3 ticks

@Mattriks
Copy link
Member

In Development:

@bjarthur
Copy link
Member Author

bjarthur commented Jun 2, 2018

@Mattriks have changed all your suggestions. thanks for the thorough review!

the problem with the plot with the missing points is that the id number for elements is randomly generated. when including multiple SVGs in the same HTML document they can conflict (ie be the same). the hack was to change the seed. long term we should look into longer random numbers, or creating a name space if possible.

i've started on a new "Thumbnail" section in the gallery, modeled after seaborn, but will put that in a separate PR as i don't want to hold this up anymore.

will merge this PR once CI passes.

@bjarthur bjarthur merged commit 688b76c into GiovineItalia:master Jun 2, 2018
@bjarthur bjarthur deleted the bja/docs branch June 2, 2018 11:27
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.

5 participants