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

Allow using apply method with method strings #3554

Merged
merged 9 commits into from Mar 22, 2019

Conversation

Projects
None yet
2 participants
@philippjfr
Copy link
Contributor

commented Mar 12, 2019

Implements the suggestion outlined in pyviz/datashader#723. This allows the apply method to accept strings which refer to methods on the object, e.g.:

color_picker

sample_apply

After some discussion @jlstevens and I have decided to make .apply a namespace accessor with the following convenience methods:

  • .apply.opts
  • .apply.select
  • .apply.reduce
  • .apply.aggregate

This also means we can deprecate HoloMap.reduce and HoloMap.sample which are outliers in our API (which are inherited but are not actually usable on DynamicMap) and can be achieved by chaining apply and collapse methods.

@philippjfr philippjfr force-pushed the apply_method_via_string branch from d7677e9 to c01f443 Mar 22, 2019

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@jlstevens Ready to review.

Show resolved Hide resolved holoviews/core/accessors.py Outdated
Show resolved Hide resolved holoviews/core/accessors.py Outdated
@jlstevens

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Other than two minor suggestions (_args -> _function_args and redim -> Redim) I am very happy with the PR and pleased to see the accessors live in a separate file. Happy to merge once the renaming is done and the tests are green.

philippjfr added some commits Mar 22, 2019

@philippjfr philippjfr force-pushed the apply_method_via_string branch from 3e9e14d to ff3ee9d Mar 22, 2019

@jlstevens

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Tests are passing now and I'll go ahead and merge. Excited to try it out!

@jlstevens jlstevens merged commit 52548a6 into master Mar 22, 2019

5 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
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.2%) to 90.222%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the apply_method_via_string branch Apr 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.