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

Add bokeh statistics operations, elements and plots #1985

Merged
merged 40 commits into from Oct 31, 2017
Merged

Conversation

@philippjfr
Copy link
Member

@philippjfr philippjfr commented Oct 9, 2017

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

@philippjfr philippjfr force-pushed the bokeh_stats_elements branch from bf566ca to c10af53 Oct 30, 2017
@philippjfr philippjfr force-pushed the bokeh_stats_elements branch from 99114c0 to 64ed9cc Oct 30, 2017
@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Oct 31, 2017

Docstrings, element references and unit tests left, should be able to finish that up in the morning. Compositor now handles everything, and I'm very happy with it overall. It did touch an awful lot of files though.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Oct 31, 2017

@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.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Oct 31, 2017

Almost a 1% coverage increase! Ready for final review and merge whenever.

from .chart import Chart, Scatter


class _StatisticsElement(Chart):

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2017
Contributor

I would still prefer to have this called StatisticalElement which matches the name used in the unit testing class.

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2017
Author Member

Sure, don't want that appearing in the top-level namespace though.

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2017
Contributor

Can't we just set __all__ so if it is used it has to be explicitly imported?

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2017
Author Member

I'm now filtering abstract classes from hv.element. Works fine.

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)):

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2017
Contributor

How come we didn't need this before when the compositor was used?

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2017
Author Member

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.

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2017
Contributor

Ok, removing this would be good in that case.

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])

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2017
Contributor

Maybe this stuff should be provided by Compositor itself? (i.e CompositeOverlay vs Element)

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2017
Contributor

I also think this should be done by compositor as you are accessing an underscore attribute, namely _pattern_spec.

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2017
Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2017
Contributor

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.

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2017
Author Member

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.

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2017
Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2017
Contributor

Just checking that this function is confined to core....

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2017
Author Member

Good thing to check but I'm fairly certain.

This comment has been minimized.

@philippjfr

philippjfr Oct 31, 2017
Author Member

Yes it is.

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.""")

This comment has been minimized.

@jlstevens

jlstevens Oct 31, 2017
Contributor

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?

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Oct 31, 2017

After a few more comments above are addressed, I'm happy to merge.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Oct 31, 2017

Okay all comments addressed, just waiting on tests now.

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Oct 31, 2017

Tests have passed. Merging!

@jlstevens jlstevens merged commit 77b92f0 into master Oct 31, 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 increased (+0.9%) to 80.739%
Details
@philippjfr
s3-reference-data-cache Test data is cached.
Details
@philippjfr philippjfr added this to the v1.9 milestone Nov 2, 2017
@philippjfr philippjfr deleted the bokeh_stats_elements branch Nov 2, 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