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

Docs and minor code fixes #1765

Merged
merged 11 commits into from
Jul 31, 2017
Merged

Conversation

ea42gh
Copy link
Contributor

@ea42gh ea42gh commented Jul 29, 2017

I made the changes suggested in #1696 that I was comfortable with.
I left out changes that would modify the structure of the documents.

@@ -97,11 +99,11 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"Exploring a parameter space using widgets is one of the most flexible approaches but as we noted above it also makes it difficult to compare between different parameter values. HoloViews therefore supplies several container objects, which behave similarly to a ``HoloMap`` but have a visual representation:\n",
"Exploring a parameter space using widgets is one of the most flexible approaches but as we noted above it also makes it difficult to compare between different parameter values. HoloViews therefore supplies several container objects, which behave similarly to a ``HoloMap`` but have a different visual representation:\n",
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 the idea here was not that the visual representation is different, but that there was one at all. HoloMaps simply punt on the idea of representing the additional dimensions visually, instead giving a widget so that the user is queried about which value they want to see. Other containers actually show values along that dimension simultaneously. Maybe you can see how to re-word it so that it would be both correct and clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes much more sense! How about
'a visual representation showing the effect of all parameter values simultaneously'

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

"labels_anchors": false,
"latex_user_defs": false,
"report_style_numbering": false,
"user_envs_cfg": false
}
Copy link
Member

Choose a reason for hiding this comment

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

I see that it's possible for Jupyter to fill notebooks with even more junk than they usually do! You might consider running the stripping command listed in #1617 (comment) , or else this can be merged and we can run that separately.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, looking through it, it would be very helpful if you could run such a stripping function on this PR, so that the real changes can be seen more clearly; most of the changes look like Jupyter's auto-generated fluff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK!

@jbednar
Copy link
Member

jbednar commented Jul 29, 2017

Looks good apart from minor comments above; thanks!

@@ -463,7 +463,7 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"Once data is in columnar form, it is simple to apply a variety of operations. For instance, Dataset can be sorted by their dimensions using the ``.sort()`` method. By default, this method will sort by the key dimensions, but any other dimension(s) can be supplied to specify sorting along any other dimensions:"
"Once data is in columnar form, it is simple to apply a variety of operations. For instance, Dataset can be sorted by their dimensions using the ``.sort()`` method. By default, this method will sort by the key dimensions, but any other dimension(s) can be sorted by providing an them as an argument list to the sort method:"
Copy link
Member

Choose a reason for hiding this comment

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

providing them

@@ -487,7 +487,7 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"Data is often grouped in various ways, and the Dataset interface provides various means to easily compare between groups and apply statistical aggregates. We'll start by generating some synthetic data with two groups along the x-axis and 4 groups along the y axis."
"Data is often grouped in various ways, and the Dataset interface provides various means to easily compare between groups and apply statistical aggregates. We'll start by generating some synthetic data with two groups along the x-axis and 4 groups along the y-axis."
Copy link
Member

Choose a reason for hiding this comment

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

"y-axis" would be an adjective, as in "y-axis label". It's correct with no -, because it's used as a noun here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so should it be 'along the x axis and 4 groups along the y axis.'
(no hyphen on either)

Copy link
Member

Choose a reason for hiding this comment

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

Right. I try to train people about how to use hyphens, but no one cares about it but me. :-)

Copy link
Contributor Author

@ea42gh ea42gh Jul 30, 2017

Choose a reason for hiding this comment

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

Actually your comment had me intrigued: thanks for the link!
Re code changes, they were a better set of tick marks, a larger marker size,
and in one instance, better alignment (one of my pet peeves, I want to
see my indices at a glance), e.g.,

b = a[0][i-1] * a[1][i-1] +
    a[0][i  ] * a[1][i  ]

@@ -23,18 +23,18 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"HoloViews objects provide a convenient way of wrapping your data along with some metadata for exploration and visualization. For the simplest visualizations, you can simply declare a small collection of element which can then be composed or placed in an appropriate container. As soon as the task becomes more complex, it is natural to write a function of a class to output HoloViews objects.\n",
"HoloViews objects provide a convenient way of wrapping your data along with some metadata for exploration and visualization. For the simplest visualizations, you can simply declare a small collection of elements which can then be composed or placed in an appropriate container. As soon as the task becomes more complex, it is natural to functions that output HoloViews objects.\n",
Copy link
Member

Choose a reason for hiding this comment

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

natural to write functions that

@jbednar
Copy link
Member

jbednar commented Jul 30, 2017

Looks great! I haven't checked what the effects of the minor changes made to the code were, but the docs look ready to merge after the above tiny fixes. Thanks!

@jlstevens
Copy link
Contributor

@jbednar If you agree that all your comments have been addressed, I'm happy to merge!

@ea42gh
Copy link
Contributor Author

ea42gh commented Jul 31, 2017

Hold off a little more: I am currently on 14_LargeData
OK, 14 is submitted as well. Won't get to do more for a couple of days...

@jlstevens
Copy link
Contributor

Sure. Just shout when you think it is ready!

@ea42gh
Copy link
Contributor Author

ea42gh commented Jul 31, 2017

I won't add anything further for a few days. Just do what is most convenient for you...

@jlstevens
Copy link
Contributor

In that case I think we should merge if @jbednar signs off on the latest changes.

@jbednar jbednar merged commit aeccd50 into holoviz:master Jul 31, 2017
@jbednar
Copy link
Member

jbednar commented Jul 31, 2017

Looks great, thanks!

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

3 participants