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

Handle batched styles consistently #1241

Merged
merged 10 commits into from Apr 4, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Apr 3, 2017

The handling of batched style options was handled differently on all the different plots. As I outlined in #795 this should be handled consistently and this PR makes that happen.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 3, 2017

Sounds like a good idea. Do you think it is possible to set up tests to try and enforce consistency between batched and non-batched?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 3, 2017

Do you think it is possible to set up tests to try and enforce consistency between batched and non-batched?

Yes, I'll add some of those now. That said there's certain cases that don't work yet, which I'll list here once I'm done.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 3, 2017

As far as I can tell there's only one style option that doesn't work in batched mode, which is marker. Definitely annoying but a huge improvement because we actually only handled colors correctly before.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 3, 2017

While the lack of support for marker is annoying and is something I will file with bokeh, every single other option now works correctly in batched mode including selection and nonselection colors/alpha, which I'm pretty happy about. Also added some tests, ready to review/merge.

mapping. Supplying nvals adds the style entry multiple times, added
as an array by default. When multiple is active multiple scalar values
will be added.
"""

This comment has been minimized.

@jlstevens

jlstevens Apr 3, 2017

Member

Is there a simple illustrative example you could put in the docstring? I have trouble imagining what it does from this description alone...

This comment has been minimized.

@philippjfr

philippjfr Apr 3, 2017

Member

Yeah, it's fairly complex I'll try to come up with some examples, since it supports three modes of expanding styles corresponding to Path, Points, and Curve types.

This comment has been minimized.

@jlstevens

jlstevens Apr 3, 2017

Member

Actually, in that case it would just be good to have a nice set of unit tests, starting simple and getting more complicated, covering all three modes. This is one of those cases where I feel unit tests can almost substitute docstrings...

This comment has been minimized.

@philippjfr

philippjfr Apr 3, 2017

Member

There are unit tests, you can comment on their clarity though.

This comment has been minimized.

@philippjfr

philippjfr Apr 3, 2017

Member

They are more integration tests, not testing the utility but what actually ends up on the datasource.

This comment has been minimized.

@jlstevens

jlstevens Apr 3, 2017

Member

They are more integration tests, not testing the utility but what actually ends up on the datasource.

Sure. These are definitely useful and implicitly tests expand_batched_style.

What I'm saying is that explicit unit tests for expand_batched_style wouldn't hurt and would also help give some concrete examples of what it does.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 4, 2017

I simplified the utility a fair bit and added some explicit unit tests. Ready to merge when tests passing.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 4, 2017

Thanks for the new unit tests and I'm glad you managed to simplify the utility. Tests are passing.

Merging.

@jlstevens jlstevens merged commit 780a0c9 into master Apr 4, 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 increased (+0.03%) to 78.177%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the bokeh_batched_props branch Apr 11, 2017

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