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

Projects
None yet
2 participants
@jlstevens
Member

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

This comment has been minimized.

Member

philippjfr commented Apr 17, 2017

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

jlstevens commented Apr 18, 2017

@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

This comment has been minimized.

Member

jlstevens commented Apr 18, 2017

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

image

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 18, 2017

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

This comment has been minimized.

Member

philippjfr commented Apr 18, 2017

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

This comment has been minimized.

Member

jlstevens commented Apr 18, 2017

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 jlstevens added the ready label Apr 18, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 19, 2017

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

This comment has been minimized.

Member

philippjfr commented Apr 19, 2017

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

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 decreased (-0.02%) to 79.043%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the deprecate_sampled_mode branch Apr 19, 2017

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