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

ENH/VIS: Dataframe bar plot can now handle align keyword properly #6691

Merged
merged 1 commit into from
Mar 26, 2014

Conversation

sinhrks
Copy link
Member

@sinhrks sinhrks commented Mar 23, 2014

Closes #4525.

I modified the BarPlot internal alignment to meet line coordinates. Previously, BarPlot doesn't pass align keyword to matplotlib (thus matplotlib uses align='edge'), but it adjusts bar locations to looks like align='center'.

Now BarPlot pass align=center to matplotlib by default and ticks are being locates on integer value starts from 0 (0.0, 1.0 ...). Drawing bar and line on the same axes looks like below.

Output using current master:
align_incorrect

Output after fix
align_correct

self.assertEqual(ax.xaxis.get_ticklocs()[0],
ax.patches[0].get_x() + ax.patches[0].get_width())

ax = df.plot(kind='bar', width=0.9, grid=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to remove this instead of replacing with

axes = self._check_bar_alignment(df, kind='bar', width=.9, stacked=False)

?

EDIT: Nevermind, it's further down. Github tricked me.

@TomAugspurger
Copy link
Contributor

Thanks for this.

We may want a little warning in the release notes that this will change the look of the default output. I'm onto sure how guarantee plots being identical from one version to another.

You have a few magic numbers in the _check_bar_alignment function, like in tickoffset = width * (position - 0.5) + p.get_width() * 1.5. Are the .5 and 1.5 just constants that matplotlib uses?

I had a couple of notes inline, but overall this looks good. If you look at those and rebase I'll get it merged. Thanks.

@sinhrks
Copy link
Member Author

sinhrks commented Mar 25, 2014

Thanks for your confirmation. I added decorators / warning description in what's new txt, and rebased.

Fixed numbers in _check_bar_alignment is derived from BarPlot class logic (line 1690 of plotting.py).

  • width * (position - 0.5) derived from position 's default (0.5). Because BarPlot now specifies the center-bottom coordinate of bar, no adjustment should be done when position is 0.5. Thus, it should be substituted.
  • p.get_width() * 1.5 is derived from previous version's logic. BarPlot has draw non-stacked plot starting from (i +1) * width location (line 1766 of plotting.py). Now we have to specify the bar location, it should be changed to (i + 1.5) * width. Thus, to get the same offset as the previous logic, I used 1.5 to get the offset.

@jreback jreback added this to the 0.14.0 milestone Mar 25, 2014
- `position`: Specify relative alignments for bar plot layout. From 0 (left/bottom-end) to 1(right/top-end). Default is 0.5 (center). (:issue:`6604`)


.. warning::
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be here, instead in the plotting.rst (if where you show how to work with this option, if necessary)

@sinhrks
Copy link
Member Author

sinhrks commented Mar 25, 2014

OK. I have understood to write a warning about backward-compatibility in release note, but it should be in plotting.rst, or simply removed? I think usage is not necessary because the align keyword works as the same as matplotlib.

I'll correct my typo once confirmed where the description should be (or removed).

@jreback
Copy link
Contributor

jreback commented Mar 25, 2014

ok so move both the release note and the v0.14.0 note to the API section - change from an actual warning block to just text in the note (no need for a separate warning block)

@sinhrks
Copy link
Member Author

sinhrks commented Mar 26, 2014

Sure. Descriptions are under API Changes section, and corrected typo and warning block.

@jreback
Copy link
Contributor

jreback commented Mar 26, 2014

ok by me... @TomAugspurger if ok, then go ahead and merge

TomAugspurger pushed a commit that referenced this pull request Mar 26, 2014
ENH/VIS: Dataframe bar plot can now handle align keyword properly
@TomAugspurger TomAugspurger merged commit 110406c into pandas-dev:master Mar 26, 2014
@TomAugspurger
Copy link
Contributor

Looks good. Thanks

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

Successfully merging this pull request may close these issues.

make the align keyword work like matplotlib in bar plots
3 participants