Skip to content
This repository was archived by the owner on Mar 22, 2026. It is now read-only.

Fix fill randomly not working in geom_bar + geom_histogram#567

Open
jdanbrown wants to merge 1 commit into
yhat:masterfrom
jdanbrown:fix-aes-renames-order
Open

Fix fill randomly not working in geom_bar + geom_histogram#567
jdanbrown wants to merge 1 commit into
yhat:masterfrom
jdanbrown:fix-aes-renames-order

Conversation

@jdanbrown

@jdanbrown jdanbrown commented Nov 23, 2016

Copy link
Copy Markdown
Contributor

This code randomly produces one of two graphs:

ggplot(diamonds, aes(x='clarity', fill='cut')) + geom_bar()
sometimes good sometimes bad
fig-20161123t060033416866 fig-20161123t052330808668

Problem: geom._get_plot_args renames keys in mpl_params according to _aes_renames, but not in a deterministic order. In particular, geom_bar and geom_histogram both include renames:

  • color -> edgecolor
  • fill -> color

which when done in that order are fine but in the other order causes color to get dropped.

Solution: use OrderedDict in geom_bar and geom_histogram to ensure that the color -> edgecolor rename happens before the fill -> color rename.

This code randomly produces one of two graphs:
```py
ggplot(diamonds, aes(x='clarity', fill='cut')) + geom_bar()
```

| randomly good | randomly bad
|---|---
| ![fig-20161123t060033416866](https://cloud.githubusercontent.com/assets/627486/20552470/3b6e98a6-b100-11e6-9159-f3e966c58840.png) | ![fig-20161123t052330808668](https://cloud.githubusercontent.com/assets/627486/20552471/3e9ef53e-b100-11e6-8c33-4f9eb114f3c3.png)

Problem: `geom._get_plot_args` renames keys in `mpl_params` according to
`_aes_renames`, but not in a deterministic order. In particular,
`geom_bar` and `geom_histogram` both include renames:

- `color` -> `edgecolor`
- `fill` -> `color`

which when done in that order are fine but in the other order causes
`color` to get dropped.

Solution: use `OrderedDict` in `geom_bar` and `geom_histogram` to ensure
that the `color` -> `edgecolor` rename happens before the `fill` ->
`color` rename.
@jdanbrown jdanbrown force-pushed the fix-aes-renames-order branch from fec1a60 to ca1e630 Compare November 23, 2016 06:18

@DSLituiev DSLituiev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ggplot/geoms/geom.py, line 73 .items() need to be replaced with six.iteritems for backward compatibility

@jdanbrown

Copy link
Copy Markdown
Contributor Author

Good call, but there are two other occurrences of .items() in this file that this change doesn't touch, and this change only turns one existing .items() into another, so what do you think about calling six.iteritems out of scope for this PR to keep the diff minimal (e.g. easier rebasing)?

@DSLituiev

Copy link
Copy Markdown

let us see what yhat people say. Opened issue #578.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants