-
Notifications
You must be signed in to change notification settings - Fork 250
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
update index, tutorial, and manual #1173
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1173 +/- ##
=========================================
Coverage ? 87.32%
=========================================
Files ? 34
Lines ? 4080
Branches ? 0
=========================================
Hits ? 3563
Misses ? 517
Partials ? 0
Continue to review full report at Codecov.
|
9479107
to
f213dce
Compare
this is ready to merge i think. could someone please review? in particular, please let me know if the tutorial and manual are now amenable to a simple translation into a jupyter notebook for a video tutorial. i tried to work them towards this direction. one thing i need to fix is the strange "Plot(...)Plot(...)" in the output when building the docs. i'll be completely offline from august 2-15th, and plan on digging into 0.7 when i get back. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the tutorial and manual changes. The manual looks more clear with these changes and the tutorial looks great. It should be easy to convert these to notebooks for a video tutorial. :)
docs/src/tutorial.md
Outdated
mapping "aesthetics" to columns in the data frame, and this is followed by some | ||
number of elements, which are the nouns and verbs, so to speak, that form the | ||
The first argument is the data to be plotted, the keyword arguments at the end | ||
map "aesthetics" to columns in the data frame, and everything in between is some |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence was a little long/hard for me to read on a first pass. Perhaps consider breaking it up as
The first argument is the data to be plotted and the keyword arguments at the end map "aesthetics" to columns in the data frame. All input arguments between
data
andmapping
are some number of "elements", which are the nouns and verbs, so to speak, that form the grammar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great update overall, I only have a couple minor comments.
docs/src/man/themes.md
Outdated
## Examples | ||
The constructor for `Theme` takes zero or more keyword arguments each of which | ||
overrides the default value of the corresponding field. See the Theme entry in | ||
the [Library](@ref Gadfly) for a full list of keywords. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe have a direct link to Theme's docstring here?
docs/src/tutorial.md
Outdated
mapping "aesthetics" to columns in the data frame, and this is followed by some | ||
number of elements, which are the nouns and verbs, so to speak, that form the | ||
The first argument is the data to be plotted, the keyword arguments at the end | ||
map "aesthetics" to columns in the data frame, and everything in between is some |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second this.
docs/src/tutorial.md
Outdated
``` | ||
|
||
Note that there is also no need to specify `Geom.point`, as that will be | ||
supplied automatically if no other geometries are given. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be recommending this behavior? IMO it's better to be explicit about what we're plotting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we ever removed Geom.point as a default, then we should make this change. otherwise best to document it i think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could say something along the lines of:
In cases where no explicit geometry is provided, the plot will use
Geom.point
by default.
This way, it doesn't sound like we're recommending this behavior, but it does document that this is the default.
docs/src/tutorial.md
Outdated
plot(elements::Element...; mapping...) | ||
``` | ||
|
||
Here, the keyword arguments directly supply the data to be ploted, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ploted
Should be plotted.
instead of using them to indicate which columns of a DataFrame to use.
Not sure if this adds anything
``` | ||
|
||
Note that with the Array interface, extra elements must be included to specify the | ||
axis labels, whereas with a DataFrame they default to the column names. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that with the Array interface, extra elements must be included to specify the axis labels, whereas with a DataFrame they default to the column names.
What about:
Note that with the Array interface, the axis labels cannot be directly inferred so include explicit calls to Guide.xlabel
etc to specify the axis names.
|
||
""" | ||
$(FIELDS) | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
This will make keeping the docs in sync much easier and avoid issues like #852
src/theme.jl
Outdated
|
||
""" | ||
$(FIELDS) | ||
""" | ||
@varset Theme begin | ||
# If the color aesthetic is not mapped to anything, this is the color that | ||
# is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the comments are unnecessary with the docstring update and we should remove them.
src/varset.jl
Outdated
end | ||
"doc struct" | ||
@foo(Foo, "doc var", x, Int) | ||
=# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this macro used?
88d35de
to
7178ec7
Compare
thanks for the reviews! will merge once CI passes. |
2449844
to
f27b11a
Compare
merged! @xorJane what are the next steps to create a video tutorial? |
@bjarthur I'm so sorry I missed this! Now that you already have content, the steps are just to schedule the tutorial, show you how to stream/check your internet connection prior to that date, and then stream/talk through the tutorial on the scheduled date. If you have a sense of what dates might be good for you, let me know and we can put up announcements on julialang.org/learning as well as discourse, etc. Do you have access to a stable ethernet connection? Upload speeds over wifi have been pretty consistently bad/unreliable for previous tutorials, and then we see a lot of buffering and resulting frustration. :) |
And thank you again!! |
plot
signatures in the tutorial were way out of date.subplot_grid
to the manual.Theme
out of the text in the themes section.once done it should be straightforward to convert the existing docs into a jupyter notebook for a video with @xorJane
closes #1161