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

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

@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 commented Oct 31, 2017

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

from .chart import Chart, Scatter


class _StatisticsElement(Chart):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jlstevens jlstevens Oct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jlstevens jlstevens Oct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thing to check but I'm fairly certain.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jlstevens jlstevens Oct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@philippjfr
Copy link
Member Author

Okay all comments addressed, just waiting on tests now.

@jlstevens
Copy link
Contributor

Tests have passed. Merging!

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants