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 an apply method to apply functions to elements #3474

Merged
merged 16 commits into from
Mar 7, 2019
Merged

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Feb 7, 2019

This PR implements the outcome of our API discussion to provide a more convenient API than hv.util.Dynamic. The main changes are twofold:

  • .apply now defaults to ViewableElement for its spec keyword, which means it behaves exactly like an Operation
  • Added support for supplying streams and keywords to .apply, which make it behave like hv.util.Dynamic, i.e. it provides support for chaining dynamic operations, but also making existing objects become dynamic if there is a stream or parameter dependency declared.

@philippjfr philippjfr added tag: API type: enhancement Minor feature or improvement to an existing feature labels Feb 7, 2019
@philippjfr
Copy link
Member Author

This is now working as I would like it to, the main problem now is that I've had to modify the default behavior slightly to get what we want since simply setting the specs to ViewableElement isn't quite the semantics we need. The difference between .map and the broadcasting that an operation does is that an operation stops early. For example when you have an Overlay of Curves the operation will stop when it encounters the Overlay, while map will apply to the Overlay and then continue traversing down to each Curve. I now have special handling for that when specs='default', but I wonder if I should add a flag that allows getting the early stopping behavior without special handling for specs='default'.

@jlstevens
Copy link
Contributor

I dunno if this PR is ready yet but one thing I would like to see before merging is a concrete example of how to use map to effectively chain operations (with a screenshot).

Alternatively (or probably more sensibly) some examples in the documentation.

@philippjfr philippjfr added this to the v1.12.0 milestone Feb 13, 2019
@philippjfr
Copy link
Member Author

philippjfr commented Mar 6, 2019

Here is a little example of .apply in action:

from scipy import ndimage

def image(freq=0):
    xs, ys = np.mgrid[-1:1:0.05, -1:1:0.05]*freq
    return hv.Image(np.sin(xs)*np.cos(ys))
    
dmap = hv.DynamicMap(image, kdims=('freq', 'Frequency')).redim.range(freq=(1, 10))

# Define an operation which applies gaussian filtering
def filter(obj, sigma, mode):
    return obj.clone(ndimage.gaussian_filter(obj.data, sigma, mode=mode))

# Define panel widgets which will control the function sigma and mode arguments
sigma = pn.widgets.FloatSlider(name='Sigma', start=1, end=10)
mode = pn.widgets.Select(name='Mode', value='mirror', options=['reflect', 'constant', 'mirror', 'nearest', 'wrap'])

# Map widget parameters to function arguments
filtered = dmap.apply(filter, sigma=sigma.param.value, mode=mode.param.value)

# Make a panel layout
hv_panel = pn.panel(filtered)
pn.Row(hv_panel[0], pn.Column(hv_panel[1], sigma, mode))

map_example

and here is a little example using param instances in operations:

import colorcet as ct

class Example(param.Parameterized):
    
    cmap = param.Selector(default=ct.bgy, objects={'bgy': ct.bgy, 'rainbow': ct.rainbow})

example = Example()
    
points = hv.Points(np.random.randn(10000000, 2))

pn.Row(datashade(points, cmap=example.param.cmap), example.param)

screen shot 2019-03-06 at 7 42 46 pm

@philippjfr
Copy link
Member Author

Tests are failing because the unpickling doesn't seem to be working:

AttributeError: 'Box' object has no attribute '_instance__params'

I tested it locally without issues, so don't quite know what's up with that.

@jlstevens
Copy link
Contributor

I'm very happy with that API and I reviewed the code earlier. Happy to merge when the tests pass.

@jbednar
Copy link
Member

jbednar commented Mar 6, 2019

That all looks great, though I'm not sure this is really a map function; seems more like apply, in that it accepts arguments and calls the function with those arguments. It's not purely apply because there is an extra argument snuck in there that in this case consists of the current value of the dmap. If called on a HoloMap that extra argument works out to it being worthy of the name map, in that the function then gets applied to every item in the container, as map would indicate. But if called on a DynamicMap its more like a dynamic apply call than a map, and if called on an element it's more like a regular apply call (just with this extra implicit argument). So I guess it can work both ways; up to @philippjfr.

@philippjfr philippjfr changed the title Generalized map method to allow dynamic chaining Add an apply method to apply functions to elements Mar 7, 2019
@philippjfr
Copy link
Member Author

philippjfr commented Mar 7, 2019

Okay I'm actually much happier with this, .apply is now much simpler than it was when I was forcing the functionality into .map and it doesn't let you do as many stupid things. It also now neatly replaces the code that broadcasts an operation across a nested container, which eliminates some code there.

I'll work on a few more tests but it's working again, now with the .apply API.

@jbednar
Copy link
Member

jbednar commented Mar 7, 2019

Does the example above still work, after replacing .map with .apply?

@philippjfr
Copy link
Member Author

Does the example above still work, after replacing .map with .apply?

I'll push another fix shortly but yes, that should work.

@philippjfr
Copy link
Member Author

I've just pushed a few fixes and a bunch more tests, so this should be ready to merge now.

@philippjfr
Copy link
Member Author

Let's ignore the Windows failure for now, it's some regression in the latest dask 1.1.3 release which I'll have to look into independently.

@jlstevens
Copy link
Contributor

jlstevens commented Mar 7, 2019

The suggestion for updating .map is now mentioned in #3544. As the tests were passing earlier (except for Windows), I'll go ahead and merge.

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 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tag: API type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants