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

Fixed bokeh BarPlot implementation #1405

Merged
merged 2 commits into from May 5, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented May 4, 2017

We've been discussing the Bars element quite a lot recently and that its behavior was confusing. Having had to use Bars extensively I now realize the reason for that is that the current implementation doesn't really make any sense and is completely inconsistent with what the matplotlib version does.

While we will deprecate this bokeh charts plot implementation in favor of our own very soon, I still think this is worth fixing for the time being. In future we have plans to allow grouping and stacking of Bars by overlaying them. However as we move away from a bokeh charts based interface the behavior it has now is probably reasonable for a transitional period while we solve issues surrounding categorical axes in the matplotlib backend and issues surrounding nested coordinate systems in bokeh. The matplotlib plot should still be simplified to disallow simultaneous grouping and stacking, resulting in a horrendously complicated plotting class.

philippjfr added some commits May 4, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 5, 2017

I'm all in favor of simplifying the barplot implementations. Happy to merge if you think it is ready and the tests pass.

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 5, 2017

The main worry is probably backwards compatibility...

@philippjfr

This comment has been minimized.

Member

philippjfr commented May 5, 2017

The main worry is probably backwards compatibility...

I've made sure that it still behaves as before for the simple case and for the stacking case. Any other combinations were simply wrong and buggy before (probably part of the reason you complained about it being unintuitive before).

@philippjfr

This comment has been minimized.

Member

philippjfr commented May 5, 2017

I've opened an issue to summarize my conclusions about BarPlots.

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 5, 2017

Not sure there are any new unit tests to write. If you are done, I'm happy to merge as the tests are green.

@philippjfr

This comment has been minimized.

Member

philippjfr commented May 5, 2017

Not sure there are any new unit tests to write. If you are done, I'm happy to merge as the tests are green.

Bars are tested as part of the Bokeh_Elements tutorial so yes please merge.

@jlstevens jlstevens merged commit ea1e817 into master May 5, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 79.044%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the bokeh_bar_fix branch May 25, 2017

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