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

style.bar: add support for axis=None (tablewise application instead of rowwise or columnwise) #21548

Merged
merged 4 commits into from Aug 30, 2018

Conversation

Projects
None yet
4 participants
@soxofaan
Copy link
Contributor

commented Jun 19, 2018

  • eliminate code duplication related to style.bar with different align modes

  • add support for axis=None

  • fix minor bug with align 'zero' and width < 100

  • make generated CSS gradients more compact

  • Closes #21525

  • tests added / passed

  • passes git diff origin/master -u -- "*.py" | flake8 --diff

  • whatsnew entry

@soxofaan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2018

example with axis=0 (top) and axis=None (bottom):
screen shot 2018-06-20 at 01 11 39

@soxofaan soxofaan changed the title style.bar: add support for axis=None style.bar: add support for axis=None (tablewise application instead of rowwise or columnwise) Jun 20, 2018

@soxofaan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2018

related for style.background_gradient PR: #21259

@codecov

This comment has been minimized.

Copy link

commented Jun 20, 2018

Codecov Report

Merging #21548 into master will increase coverage by <.01%.
The diff coverage is 97.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #21548      +/-   ##
==========================================
+ Coverage   92.05%   92.05%   +<.01%     
==========================================
  Files         169      169              
  Lines       50733    50736       +3     
==========================================
+ Hits        46702    46706       +4     
+ Misses       4031     4030       -1
Flag Coverage Δ
#multiple 90.46% <97.5%> (ø) ⬆️
#single 42.24% <5%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/io/formats/style.py 96.42% <97.5%> (+0.26%) ⬆️

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 68273a7...e32c333. Read the comment docs.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

can you search the issues to see if we have something an for this? IIRC we do

@soxofaan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2018

@jreback did a quick search but couldn't find anything yet

note to self: #21526 should be straightforward to include in another iteration of this issue

@soxofaan soxofaan force-pushed the soxofaan:style_bar_axis_none branch from 9a0362f to 6a5a1e5 Jun 24, 2018

soxofaan added a commit to soxofaan/pandas that referenced this pull request Jun 24, 2018

ENH: Styler.bar: add support for tablewise application with axis=None p…
…andas-dev#21548

- eliminate code duplication related to style.bar with different align modes
- add support for axis=None
- fix minor bug with align 'zero' and width < 100
- make generated CSS gradients more compact
@pep8speaks

This comment has been minimized.

Copy link

commented Jun 24, 2018

Hello @soxofaan! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 21, 2018 at 14:03 Hours UTC

@soxofaan soxofaan force-pushed the soxofaan:style_bar_axis_none branch from 6a5a1e5 to 55476c1 Jun 24, 2018

@soxofaan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2018

I added a commit to handle issue #21526

@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

@soxofaan can you update the original post to include Closes #21525

will try to review this sometime soon.

@soxofaan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2018

Maybe also to be considered in this pull request:
I find the align mode names mid and zero very confusing:

  • mid claims "midpoint is centered in the cell", but if all values are strictly positive or negative, this is not the case as zero is forced into the value range
  • zero is about "zero is centered in the cell", which I would have called center (because there already is left), but this sounds a lot like mid of course.

Also: the left mode can lead to poor visualisation and is a bad choice as default IMHO. The bars do not start from zero at the left but from the minimum value. When this minimum is negative (or all values are negative), you get in the territory of poor/confusing visualisation.

In my refactor, I kept the original modes to avoid "bike shedding" issues, but I think I should at least raise the issue here.
What do you think?

@soxofaan soxofaan force-pushed the soxofaan:style_bar_axis_none branch from 55476c1 to 336f4bc Jul 11, 2018

soxofaan added a commit to soxofaan/pandas that referenced this pull request Jul 11, 2018

ENH: Styler.bar: add support for tablewise application with axis=None p…
…andas-dev#21548

- eliminate code duplication related to style.bar with different align modes
- add support for axis=None
- fix minor bug with align 'zero' and width < 100
- make generated CSS gradients more compact
@soxofaan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

(rebased)

Color the background ``color`` proportional to the values in each
column.
Draw bars in the cell backgrounds, with a width proportional to
the values

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Jul 12, 2018

Contributor

First sentence should be less than 80 characters and end with a .

Excludes non-numeric data by default.
Parameters
----------
subset: IndexSlice, default None
a valid slice for ``data`` to limit the style application to
axis: int
axis: int or None

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Jul 12, 2018

Contributor

"or None" -> "optional"

@@ -1127,40 +1064,35 @@ def bar(self, subset=None, axis=0, color='#d65f5f', width=100,
- 'mid' : the center of the cell is at (max-min)/2, or
if values are all negative (positive) the zero is aligned
at the right (left) of the cell
vmin: float or None: minimum bar value, defining the left hand limit

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Jul 12, 2018

Contributor

"or None" -> "optional"

Description should start on the next line.

https://pandas.pydata.org/pandas-docs/stable/contributing_docstring.html

@@ -350,10 +350,10 @@ def test_bar_align_left(self):
(0, 0): ['width: 10em', ' height: 80%'],
(1, 0): ['width: 10em', ' height: 80%',
'background: linear-gradient('
'90deg,#d65f5f 50.0%, transparent 0%)'],
'90deg,#d65f5f 50.0%, transparent 50.0%)'],

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Jul 12, 2018

Contributor

Why are these tests changing?

This comment has been minimized.

Copy link
@soxofaan

soxofaan Jul 12, 2018

Author Contributor

The original implementation generated gradient stops that were invalid: the positions (percentages) should not decrease. In the example above the first stop (#d65f5f) is at 50% and de second (transparent) was originally at 0%, but it's more correct to have the second one explicitly at 50% as well. (Browsers are expected to guess the correct intention of these incorrect cases, but you never know of course.)

In other test cases, the original css was also bit redudant, e.g.:

transparent 0%, transparent 10.0%, #5fba7d 10.0%

can be simplified to

transparent 10.0%, #5fba7d 10.0%

or even worse, sometimes things like these were part of the gradient: transparent 0%, transparent 0.0%, #5fba7d 0.0%, which is completely redundant.

The new implementation improves on all these points, so original tests were adapted.

This comment has been minimized.

Copy link
@soxofaan

soxofaan Jul 12, 2018

Author Contributor

And there was also a test case (test_bar_align_zero_pos_and_neg) with bar size bug in the original implementation when width < 100, which is also fixed in new implementation

@soxofaan soxofaan force-pushed the soxofaan:style_bar_axis_none branch from 9822573 to 2694732 Jul 12, 2018

soxofaan added a commit to soxofaan/pandas that referenced this pull request Jul 12, 2018

ENH: Styler.bar: add support for tablewise application with axis=None p…
…andas-dev#21548

- eliminate code duplication related to style.bar with different align modes
- add support for axis=None
- fix minor bug with align 'zero' and width < 100
- make generated CSS gradients more compact
@soxofaan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

@TomAugspurger : I addressed the docstring style remarks and rebased

@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

Thanks.

There's a linting failure: https://travis-ci.org/pandas-dev/pandas/jobs/403324861#L2928.

If you could fix that, then +1 from me.

@soxofaan soxofaan force-pushed the soxofaan:style_bar_axis_none branch from 2694732 to 706da47 Jul 26, 2018

soxofaan added a commit to soxofaan/pandas that referenced this pull request Jul 26, 2018

ENH: Styler.bar: add support for tablewise application with axis=None p…
…andas-dev#21548

- eliminate code duplication related to style.bar with different align modes
- add support for axis=None
- fix minor bug with align 'zero' and width < 100
- make generated CSS gradients more compact
@soxofaan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

@TomAugspurger done and rebased

@@ -571,6 +571,7 @@ Other
- :meth: `~pandas.io.formats.style.Styler.background_gradient` now takes a ``text_color_threshold`` parameter to automatically lighten the text color based on the luminance of the background color. This improves readability with dark background colors without the need to limit the background colormap range. (:issue:`21258`)
- Require at least 0.28.2 version of ``cython`` to support read-only memoryviews (:issue:`21688`)
- :meth: `~pandas.io.formats.style.Styler.background_gradient` now also supports tablewise application (in addition to rowwise and columnwise) with ``axis=None`` (:issue:`15204`)
- :meth: `~pandas.io.formats.style.Styler.bar` now also supports tablewise application (in addition to rowwise and columnwise) with ``axis=None`` and setting clipping range with ``vmin`` and ``vmax`` (:issue:`21548` and :issue:`21526`)

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Aug 12, 2018

Contributor

No space between :meth: and the method.

This comment has been minimized.

Copy link
@soxofaan

soxofaan Aug 14, 2018

Author Contributor

Ok, I'll fix that in the preceding two bullet points as well then

"""Draw bar chart in dataframe cells"""

# Get input value range.
smin = s.values.min() if vmin is None else vmin

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Aug 12, 2018

Contributor

What are the types of s? Series or DataFrame?

We don't want .values.min, since if it has any missing values the output will be missing. Presumably, we'll want to skip missing values here...

Pandas doesn't yet support reducing over all axes with min / max, so you'll likely need to handle series / frame separately here

This comment has been minimized.

Copy link
@soxofaan

soxofaan Aug 14, 2018

Author Contributor

Good point. Apparently the original implementation doesn't handle this properly either

@@ -1129,38 +1064,37 @@ def bar(self, subset=None, axis=0, color='#d65f5f', width=100,
at the right (left) of the cell
.. versionadded:: 0.20.0
vmin: float, optional

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Aug 12, 2018

Contributor

Can you add ..versionadded for both of these.

@TomAugspurger TomAugspurger added this to the 0.24.0 milestone Aug 12, 2018

@soxofaan soxofaan force-pushed the soxofaan:style_bar_axis_none branch from 706da47 to 44a85b7 Aug 14, 2018

soxofaan added a commit to soxofaan/pandas that referenced this pull request Aug 14, 2018

ENH: Styler.bar: add support for tablewise application with axis=None p…
…andas-dev#21548

- eliminate code duplication related to style.bar with different align modes
- add support for axis=None
- fix minor bug with align 'zero' and width < 100
- make generated CSS gradients more compact
@soxofaan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2018

@TomAugspurger done and rebased

"""Draw bar chart in dataframe cells"""

# Get input value range.
smin = s.min().min() if vmin is None else vmin

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Aug 18, 2018

Contributor

This will fail for object dtypes, which may happen in this context. Even though it's slightly wordier, I'd prefer

smin = s.min() if vmin is None else vmin
if isinstance(smin, ABCSeries):
    smin = smin.min()

to reduce it down to a scalar.

LGTM otherwise.

soxofaan added some commits Jun 19, 2018

ENH: Styler.bar: add support for tablewise application with axis=None #…
…21548

- eliminate code duplication related to style.bar with different align modes
- add support for axis=None
- fix minor bug with align 'zero' and width < 100
- make generated CSS gradients more compact

@soxofaan soxofaan force-pushed the soxofaan:style_bar_axis_none branch from 44a85b7 to 1c34755 Aug 21, 2018

@soxofaan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

@TomAugspurger done and rebased

@TomAugspurger TomAugspurger merged commit 6b83df9 into pandas-dev:master Aug 30, 2018

6 checks passed

ci/circleci: py27_compat Your tests passed on CircleCI!
Details
ci/circleci: py35_ascii Your tests passed on CircleCI!
Details
ci/circleci: py36_locale Your tests passed on CircleCI!
Details
ci/circleci: py36_locale_slow Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2018

Thanks @soxofaan!

Sup3rGeo added a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

ENH? Styler.bar support for axis=None
* ENH: Styler.bar: add support for tablewise application with axis=None pandas-dev#21548

- eliminate code duplication related to style.bar with different align modes
- add support for axis=None
- fix minor bug with align 'zero' and width < 100
- make generated CSS gradients more compact

* ENH: Styler.bar: add support for vmin/vmax pandas-dev#21526

* ENH: Styler.bar: properly support NaNs

* ENH: Styler.bar: docstring improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.