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

Raised NotImplementedError for unsupported DynamicMap methods #1262

Merged
merged 2 commits into from Apr 10, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Apr 10, 2017

As outlined in #442 the reindex, add_dimension, and drop_dimension methods on DynamicMap are simply not valid because the callback expects specific arguments which cannot be changed. Since these are inherited from MultiDimensionalMapping which is really far removed from DynamicMap it'd be a nightmare to to generate sensible baseclasses to ensure these methods don't appear at all. Therefore I'm just raising NotImplementedErrors letting the user know that they'll have to cast their DynamicMap with a cache to a HoloMap before calling the one of those methods.

Edit: Reindex does make sense as long as you're only reordering dimensions, so I've now implemented it.

@philippjfr philippjfr added the API label Apr 10, 2017

reindexed = super(DynamicMap, self).reindex(kdims, force)
def reindex(*args, **kwargs):
keymap = {kd.name: arg for kd, arg in zip(self.kdims, args)}
keymap.update(kwargs)

This comment has been minimized.

@philippjfr

philippjfr Apr 10, 2017

Member

Note: This bit is written with all kwarg callbacks in mind.

This comment has been minimized.

@jbednar

jbednar Apr 10, 2017

Member

Sounds good.

This comment has been minimized.

@jlstevens

jlstevens Apr 10, 2017

Member

You mean you are assuming Callable can be invoked with a bunch of kwargs and nothing else?

That is fine unless the callback contains *args in which case Callable needs positional arguments it passes through directly to the callback. Once my PR is merged you can easily inspect the argspec to see if varargs is set, in which case you could raise another exception.

This comment has been minimized.

@philippjfr

philippjfr Apr 10, 2017

Member

That is fine unless the callback contains *args in which case Callable needs positional arguments it passes through directly to the callback. Once my PR is merged you can easily inspect the argspec to see if varargs is set, in which case you could raise another exception.

No need, this handles both cases already.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 10, 2017

Ready to merge.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 10, 2017

Looks good and it is nice to have reindex available: it could help people improve their indexing and can also be used to change the order of widgets.

Tests are green so I'll merge now.

@jlstevens jlstevens merged commit 308528c into master Apr 10, 2017

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.3%) to 79.192%
Details
s3-reference-data-cache Tests passing no test data changes required.
Details

@philippjfr philippjfr deleted the dynamic_disable_methods branch Apr 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment