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

Fixed bokeh BarPlot implementation #1405

Merged
merged 2 commits into from May 5, 2017
Merged

Fixed bokeh BarPlot implementation #1405

merged 2 commits into from May 5, 2017

Conversation

@philippjfr
Copy link
Member

@philippjfr 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 philippjfr force-pushed the bokeh_bar_fix branch from 3acc764 to 7bea1f7 May 4, 2017
@jlstevens
Copy link
Contributor

@jlstevens 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
Copy link
Contributor

@jlstevens jlstevens commented May 5, 2017

The main worry is probably backwards compatibility...

@philippjfr
Copy link
Member Author

@philippjfr 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
Copy link
Member Author

@philippjfr philippjfr commented May 5, 2017

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

@jlstevens
Copy link
Contributor

@jlstevens 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
Copy link
Member Author

@philippjfr 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
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
@philippjfr
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants