Skip to content

Conversation

@TomAugspurger
Copy link
Contributor

@TomAugspurger TomAugspurger commented May 19, 2016

Closes #13222, #13225

  • Update docstring and notebook on output shapes matching
  • Change build process to use nbconvert to execute the notebook and then convert using NBConvert.

Second commit puts some more restrictions on the output of a user's function passed to .apply.

@TomAugspurger TomAugspurger added Docs Output-Formatting __repr__ of pandas objects, to_string IO HTML read_html, to_html, Styler.apply, Styler.applymap labels May 19, 2016
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we don't have a space before the :

? in general I mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The examples here do: https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt#sections, but I don't think it's enforced, does pandas have a preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure we are consistent as have seen both ways (or does it even format differently)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wrong - it does specify spaces on both sides
(contrast this to linting which doesn't have s space - but different issue)

Copy link
Member

Choose a reason for hiding this comment

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

yes, it definitly specify spaces on both sides, it's a comment I often give when reviewing docstrings ... :-) (and it is needed to get the html formatting correctly using numpydoc, don't know if napoleon is more flexible)

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should have a commit hook to check this? or lint check or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Last time I looked for something like this, it only checked for pep257, and not for numpydoc specific things

Copy link
Member

Choose a reason for hiding this comment

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

But there seems some discussion/work under way: PyCQA/pydocstyle#129

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that looks good / we should follow that release

@jorisvandenbossche
Copy link
Member

@TomAugspurger Did you change a lot in the notebook itself? (Or other phrased: is it worth reviewing? Because online the diff is not shown)

@TomAugspurger
Copy link
Contributor Author

@jorisvandenbossche the notebook changes were

Added:

For Styler.applymap your function should take a scalar and return a single string with the CSS attribute-value pair.
For Styler.apply your function should take a Series or DataFrame (depending on the axis parameter), and return a Series or DataFrame with an identical shape where each value is a string with a CSS attribute-value pair.

and

And crucially the input and output shapes of func must match. If x is the input then func(x).shape == x.shape.

The reason the diff is so big is that I deleted all the output from the notebook committed to the repo.

@jreback
Copy link
Contributor

jreback commented May 19, 2016

@TomAugspurger what about have a shape check when using .apply? e.g. to guarantee that its the same (or otherwise raise)?

@jreback jreback added this to the 0.18.2 milestone May 19, 2016
@TomAugspurger
Copy link
Contributor Author

@TomAugspurger what about have a shape check when using .apply

I'll push that change tonight, it's pretty easy, just need tests.

@jreback
Copy link
Contributor

jreback commented May 19, 2016

gr8! @TomAugspurger better to have an exception than documentation. Further no issues with restricting .apply. No need to make this too general (like DataFrame.apply) that just leads to blind pathways.

@TomAugspurger TomAugspurger force-pushed the styler-doc branch 2 times, most recently from 4175e4b to 375676d Compare May 20, 2016 12:13
@TomAugspurger
Copy link
Contributor Author

TomAugspurger commented May 20, 2016

Sorry for the delay, pushed changes.

  1. The result of Styler.apply(func, axis=0/1) must have the same shape. The output is from DataFrame.apply, so we know we have a Series with the correct index labels.
  2. The result of Styler.apply(func, axis=None) must be a DataFrame with the same index columns / labels (and so same shape as well).

I put the second one in place to avoid confusing output like

screen shot 2016-05-20 at 7 14 39 am

Only the top row gets colored, even though the output was 2x2.

Edit: added a whatsnew.

@TomAugspurger TomAugspurger force-pushed the styler-doc branch 3 times, most recently from 69f91cb to 6b0a9c6 Compare May 20, 2016 12:28
@TomAugspurger TomAugspurger changed the title DOC: Styler documentation changes DOC/API: Styler documentation changes Jun 16, 2016
@TomAugspurger
Copy link
Contributor Author

Sorry, forgot this was still open.

Here are the relevant changes to the notebook

Notice that the output shape of highlight_max matches the input shape, an array with len(s) items.

and

When using Styler.apply(func, axis=None), the function must return a DataFrame with the same index and column labels.

and

And crucially the input and output shapes of func must match. If x is the input then func(x).shape == x.shape.

@codecov-io
Copy link

codecov-io commented Jun 16, 2016

Current coverage is 84.33%

Merging #13225 into master will increase coverage by <.01%

@@ master #13225 diff @@ ========================================== Files 138 138 Lines 51068 51080 +12 Methods 0 0 Messages 0 0 Branches 0 0 ========================================== + Hits 43065 43080 +15  + Misses 8003 8000 -3  Partials 0 0 

Powered by Codecov. Last updated by b06bc7a...a7ff39c

doc/README.rst Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

we might also have this same documenation in contributing.rst so update there as well (as well as some info on what deps you need to build 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.

Maybe we can just include this file in contributing.rst, to not duplicate things (I thought it was done like that before)

Copy link
Member

Choose a reason for hiding this comment

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

Isn't there a .format(..) missing? (given the {!r})

@jorisvandenbossche
Copy link
Member

@TomAugspurger Does it build locally for you? (the docs) As on travis there is an error "InputError: [Errno 2] No such file or directory: 'source/html-styling.html'" (not sure if that means that something went wrong ..)

- Update docstring on output shapes matching - Change build process API: Enforce output shape requirements on Styler.apply
@TomAugspurger
Copy link
Contributor Author

Addressed all the comments, thanks. @jorisvandenbossche it does build locally, running from pandas/doc. Is that failure from before this pull request? I'll check this output of this Travis run.

@jorisvandenbossche
Copy link
Member

Let's see how the travis docs will look like after it is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Docs IO HTML read_html, to_html, Styler.apply, Styler.applymap Output-Formatting __repr__ of pandas objects, to_string

4 participants