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

Improve DynamicMap usability and deprecate sampled mode #1305

Merged
merged 39 commits into from
Apr 19, 2017

Conversation

jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Apr 17, 2017

After PRs #1238, #1260 and #1302, DynamicMaps are much more user friendly. After #1302 we realized we could also deprecate sampled mode, finishing off the job of #1238.

In particular you can now create a DynamicMap using this syntax:

hv.DynamicMap(lambda x: hv.Curve([x*1,x*2,x*3]), kdims=['x']).redim.range(x=(0,1))

But if you forget to set the ranges, an exception is currently raised. Instead, what should happen is that the repr of the DynamicMap should be shown with a warning, suggesting the use of redim.range.

This would make a basic, valid DynamicMap declaration completely analogous to that needed to declare a HoloMap: instead of HoloMap(data, kdims=['x']) you'll be able to do DynamicMap(callable, kdims=['x']). This object won't display itself (unless you follow the redim.range advice in the warning but you can visualize it by indexing (i.e sampling) it.

In addition to being more user friendly, this should make sample mode unnecessary and actually simplify the implementation. I would also like to add all extra validation to DynamicMap declarations that I can think of to make our exceptions more user friendly.

Tasks

  • Fix bug in _initial_key
  • Improve exception message when an _initial_key cannot be generated (dimension ranges lacking)
  • Raise a SkipRendering exception to show the DynamicMap repr and a suitable warning.
  • Remove sample mode
  • Add extra validation, perhaps in redim.
  • Add unit tests

@philippjfr
Copy link
Member

I'm strongly in favor of removing sampled mode and the approach taken here so far looks good. Since param just warns about undefined parameters I don't think any deprecation is needed for the sampled parameter although you may want to add an explicit deprecation warning.

@jlstevens
Copy link
Contributor Author

jlstevens commented Apr 17, 2017

Here is an example of what now happens if you try to use DynamicMap like a HoloMap and forget to set the ranges:

image

@jlstevens
Copy link
Contributor Author

@philippjfr I still want to add unit tests but otherwise I think what I currently have is ready for review.

I would have liked to add some validation in redim to complain when you try to modify non-existent dims but this seems tricky due to dynamic support. Any ideas? If there is some other sort of validation that might help users declare DynamicMap, I would be happy to implement it in this PR.

@jlstevens
Copy link
Contributor Author

For reference here is the new warning message if the DynamicMap is left unbounded:

image

@jlstevens
Copy link
Contributor Author

Maybe the message should say 'cannot be displayed directly'? or 'displayed without indexing'?

You can still generate visual display with such a DynamicMap, it just doesn't 'visualize itself' as a HoloMap would...

@philippjfr
Copy link
Member

I would have liked to add some validation in redim to complain when you try to modify non-existent dims but this seems tricky due to dynamic support.

That is quite problematic in general because redim is meant for recursing and applying where it find the dimension. Forcing it to match a dimension would be quite annoying in a lot of usecases.

@jlstevens
Copy link
Contributor Author

That is quite problematic in general because redim is meant for recursing and applying where it find the dimension. Forcing it to match a dimension would be quite annoying in a lot of usecases.

RIght. That is why I quickly gave up on my attempts at generating better exception messages for invalid redim operations.

@jlstevens
Copy link
Contributor Author

I've now added some more unit tests and I think this PR is ready to review/merge once the Travis goes green. I'm also happy to take on suggestions for other quick improvements relating to DynamicMap.

@philippjfr
Copy link
Member

Just went over it and it all looked good. I also can't think of any further improvements for now, I think it's been streamlined a lot and is now pretty solid.

@philippjfr philippjfr merged commit 3d05c84 into master Apr 19, 2017
@philippjfr philippjfr deleted the deprecate_sampled_mode branch April 19, 2017 21:38
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