Add bokeh statistics operations, elements and plots #1985
Conversation
philippjfr
added
tag: API
tag: backend: bokeh
type: feature
philippjfr
force-pushed
the
bokeh_stats_elements
branch
9 times, most recently
from
a789e03
to
e9dd4fd
Compare
philippjfr
force-pushed
the
bokeh_stats_elements
branch
from
e9dd4fd
to
bf566ca
Compare
philippjfr
force-pushed
the
bokeh_stats_elements
branch
from
bf566ca
to
c10af53
Compare
philippjfr
force-pushed
the
bokeh_stats_elements
branch
from
99114c0
to
64ed9cc
Compare
|
@jlstevens This is now ready for review. I've added element notebooks for both new Elements along with a new demo, added docstrings to the operations and added unit tests for transferring options, for the statistics elements themselves and for the compositing of the statistics elements. Should get a very healthy boost in coverage overall. |
|
Almost a 1% coverage increase! Ready for final review and merge whenever. |
holoviews/element/stats.py
Outdated
| from .chart import Chart, Scatter | ||
|
|
||
|
|
||
| class _StatisticsElement(Chart): |
There was a problem hiding this comment.
I would still prefer to have this called StatisticalElement which matches the name used in the unit testing class.
There was a problem hiding this comment.
Sure, don't want that appearing in the top-level namespace though.
There was a problem hiding this comment.
Can't we just set __all__ so if it is used it has to be explicitly imported?
There was a problem hiding this comment.
I'm now filtering abstract classes from hv.element. Works fine.
holoviews/ipython/display_hooks.py
Outdated
| if type(element) not in Store.registry[backend]: | ||
| eltype = type(element) | ||
| if (eltype not in Store.registry[backend] and | ||
| all(eltype.__name__ != d.pattern for d in Compositor.definitions)): |
There was a problem hiding this comment.
How come we didn't need this before when the compositor was used?
There was a problem hiding this comment.
I can actually remove this again, it was needed when I didn't define plotting class stubs for these elements, which I then restored because I realized options wouldn't work correctly without them.
There was a problem hiding this comment.
Ok, removing this would be good in that case.
holoviews/plotting/renderer.py
Outdated
| if any(len(c._pattern_spec) == 1 for c in Compositor.definitions): | ||
| obj = obj.map(lambda obj: Compositor.collapse_element(obj, mode='data', | ||
| backend=backend), | ||
| [Element]) |
There was a problem hiding this comment.
Maybe this stuff should be provided by Compositor itself? (i.e CompositeOverlay vs Element)
There was a problem hiding this comment.
I also think this should be done by compositor as you are accessing an underscore attribute, namely _pattern_spec.
There was a problem hiding this comment.
Maybe something like Compositor.map(obj, backend)?
| new_ids = tuple(overlay.traverse(lambda x: id(x), [spec_fn])) | ||
| if new_ids == prev_ids: | ||
| return overlay | ||
| prev_ids = new_ids |
There was a problem hiding this comment.
I am happy that we have StatisticalCompositorTest but I still don't yet have a mental model of how this code extends how Compositor works.
There was a problem hiding this comment.
I'll quickly describe it:
a) Compositors can now be applied to individual elements not just overlays.
b) Compositors are applied iteratively until there are no more matches in the compositor definitions (needed to reduce Overlays with multiple elements that should be transformed)
c) If transfer_options is true, the options are transferred from the input element to the output element. Additionally plot options that also apply to the operation are transferred there.
There was a problem hiding this comment.
Ok. I think this is all stuff to demonstrate with examples when we finally expose Compositor as a useful thing to users.
| "not %s." % (group, dims)) | ||
| dimensions[group] = [d if isinstance(d, Dimension) else Dimension(d) for d in dims] | ||
| return dimensions | ||
|
|
There was a problem hiding this comment.
Just checking that this function is confined to core....
There was a problem hiding this comment.
Good thing to check but I'm fairly certain.
holoviews/core/options.py
Outdated
| The group identifier for the output of this particular compositor""") | ||
|
|
||
| kwargs = param.Dict(doc=""" | ||
| Optional set of parameters to pass to the operation.""") | ||
|
|
||
| transfer_options = param.Boolean(default=False, doc=""" | ||
| Whether to transfer the options from the input to the output.""") | ||
|
|
There was a problem hiding this comment.
I think this is fine but maybe the part that transfers parameters (i.e plot options) to the operation should be a separate flag. Maybe transfer_parameters? or propagate_params?
|
After a few more comments above are addressed, I'm happy to merge. |
|
Okay all comments addressed, just waiting on tests now. |
|
Tests have passed. 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. |
This PR will replace the seaborn interface with a set of statistical elements, operations and plots which will all work together. For now it's very much a WIP since there's still some groundwork to be laid, but it's a good start with the basic functionality in place for Distribution and Bivariate elements and corresponding operations.
Demo notebook can be seen here