Raised NotImplementedError for unsupported DynamicMap methods #1262
Conversation
| 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) |
There was a problem hiding this comment.
Note: This bit is written with all kwarg callbacks in mind.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Ready to merge. |
|
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. |
|
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. |
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
MultiDimensionalMappingwhich 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.