Handle batched styles consistently #1241
Conversation
|
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
force-pushed
the
bokeh_batched_props
branch
from
be65937 to
c9c3e68
Compare
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. |
|
As far as I can tell there's only one style option that doesn't work in batched mode, which is |
|
While the lack of support for |
holoviews/plotting/bokeh/util.py
Outdated
| 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. | ||
| """ |
There was a problem hiding this comment.
Is there a simple illustrative example you could put in the docstring? I have trouble imagining what it does from this description alone...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
There are unit tests, you can comment on their clarity though.
There was a problem hiding this comment.
They are more integration tests, not testing the utility but what actually ends up on the datasource.
There was a problem hiding this comment.
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.
|
I simplified the utility a fair bit and added some explicit unit tests. Ready to merge when tests passing. |
|
Thanks for the new unit tests and I'm glad you managed to simplify the utility. Tests are passing. Merging. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
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.