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

HoloMap and default params #704

Closed
vascotenner opened this Issue Jun 3, 2016 · 16 comments

Comments

Projects
None yet
5 participants
@vascotenner
Contributor

vascotenner commented Jun 3, 2016

I would like to display a holomap or dynamic map, with the scrubber in the slider could start at a certain point in the middle of the slider and not at the beginning. What should be changed to support this feature?

jlstevens wrote:

my inclination would be to overload the soft_range parameter of Dimensions a bit... if you supply a value and not a tuple, that could be a default value for the Dimension

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 17, 2016

my inclination would be to overload the soft_range parameter of Dimensions a bit... if you supply a value and not a tuple, that could be a default value for the Dimension

Don't like this suggestion much, seems like it's overloading the parameter and nothing about soft_range suggests anything about default values to me. It might be okay if the type was at least the same but given that it would be a single value rather than a tuple it would also require special handling in a few places.

Only meaningful thing would be to add a default parameter to Dimensions, would warrant some discussion whether adding a new parameter is worth it in this instance though.

@jbednar

This comment has been minimized.

Member

jbednar commented Jun 17, 2016

I agree that adding a default parameter to Dimension would be the appropriate way to support this. Dimension is looking quite suspiciously like a param.Parameter at this point; can you remind me why it is not the same thing?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 17, 2016

Dimension is looking quite suspiciously like a param.Parameter at this point; can you remind me why it is not the same thing?

Conceptually they are very similar but the difference is that a parameter is declared statically on a class, while you want to supply lists of custom dimensions to an instance. Maybe I'm missing something obvious but I can't quite see how that would work using a Parameter as they are usually declared directly on the Parameterized class rather than being supplied by the user.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 17, 2016

There are two reasons I suggested using soft_range in this way:

  1. This would be a nice feature to have but isn't too critical as we have done without it up until now.
  2. I really don't want Dimension to gain more parameters if at all possible.
@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 17, 2016

Sure, but that's not really a good reason to abuse an existing parameter in this way. Semantically it makes no sense to me, there would be no way to supply both a default value and a soft-range and any code dealing with soft_range would then have to be guarded against this overloaded behavior. I see two options:

  1. Reject the feature request
  2. Add a new parameter

Personally I don't see the harm in adding parameters that make sense and provide useful functionality, and definitely prefer it to overloading an existing parameter in weird ways. Whether this particular feature is important enough to warrant the addition I'm not so sure.

@jbednar

This comment has been minimized.

Member

jbednar commented Jun 17, 2016

I remember having a discussion about Dimension vs. Parameter years ago; I just don't remember the content of it. Parameter itself doesn't know that it's on a Parameterized, and so I suspect the Parameter class would work fine as a Dimension. But it would make more sense for Dimension to be a superclass of Parameter, with Parameter adding the methods that take an obj argument. And I think the outcome of that discussion was that we want Dimensions to have Parameters, which of course then isn't possible. Still, it's frustrating that we have duplicated such fundamental semantically related concepts.

As for adding a "default" parameter to a Dimension, that seems fine to me.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 17, 2016

Sure, but that's not really a good reason to abuse an existing parameter in this way. Semantically it makes no sense to me, there would be no way to supply both a default value and a soft-range and any code dealing with soft_range would then have to be guarded against this overloaded behavior.

I agree it might not be worth the extra conditional code for what would effectively be a hidden feature.

Based on that, I don't think this feature warrants an additional parameter. I think keeping Dimension simple as possible is a priority and I would rather avoid feature creep.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 17, 2016

I remember having a discussion about Dimension vs. Parameter years ago; I just don't remember the content of it.

Unfortunately I think we didn't capture that discussion because it was in person as part of a CSNG meeting. I think we did agree that there is a shared high-level concept that could be moved into param, but then got a bit derailed in philosophical discussions to the point where we jokingly discussed a Notion as a baseclass 😧

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 17, 2016

... we jokingly discussed a Notion as a baseclass

I think it should be called Concept. ;-p

My own feeling is that default is exactly the point at which a Dimension really becomes a Parameter. They are both similar, and I agree Dimension would make sense as a superclass of Parameters.

@jbednar

This comment has been minimized.

Member

jbednar commented Jun 17, 2016

I think with either Notion or Concept as a base class, we'd need to have a further base class above that, Delusion, to make it clear that it all rests on nothing.

@jlstevens jlstevens added this to the v1.7.1 milestone Apr 24, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 24, 2017

The issue with dimension semantics has been resolved and a 'default' parameter on dimension should be fine. It shouldn't be a big job but we want to release 1.7 right now so I've assigned this to a 1.7.1 milestone.

@jbednar

This comment has been minimized.

Member

jbednar commented Apr 24, 2017

Ok, thanks. Please do add this; I'm tired of having to work around the fact that all sliders start at the left, which is not always a very useful portion of the parameter space!

@jlstevens jlstevens referenced this issue Apr 27, 2017

Open

Version 1.11 action items #1380

18 of 23 tasks complete

@jlstevens jlstevens modified the milestones: v1.7.1, v1.8 Apr 27, 2017

@philippjfr philippjfr modified the milestones: v1.9, v1.8 Jun 15, 2017

@jordansamuels

This comment has been minimized.

Contributor

jordansamuels commented Jun 17, 2017

Hello, I came across this conversation and am also interested. Does this mean that when this is available, the below will work?

t = hv.Table([(0,),(1,)], kdims=['x'])
hv.DynamicMap(lambda x: t.select(x=x), kdims=[hv.Dimension('x', range=(0,1), default=1)])

As of 1.8dev2, the code still defaults x to 0 and warns Setting non-parameter attribute default=1 using a mechanism intended only for parameters. I'm asking mostly to confirm my understanding of how this works/should work. Thanks.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 17, 2017

@jordansamuels That's right...the slider would then start at 1.

I would very much like this feature to be in 1.8 but at this point it doesn't look like that will be possible.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 30, 2017

@philippjfr I would love to have this ahead of Scipy! I don't know if it will be possible but I'll optimistically assign to 1.8.1 anyway.

@jlstevens jlstevens modified the milestones: v1.8.1, v1.9 Jun 30, 2017

@philippjfr philippjfr modified the milestones: v1.9, v1.8.1 Jul 5, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 5, 2017

Definitely not happening before SciPy. Also this is a feature and should therefore wait for a non-minor release.

@philippjfr philippjfr removed this from the v1.9 milestone Nov 3, 2017

@philippjfr philippjfr added this to the v1.10 milestone Nov 3, 2017

@philippjfr philippjfr referenced this issue Mar 4, 2018

Merged

Add support for Dimension.default #2396

3 of 3 tasks complete

@philippjfr philippjfr closed this Mar 6, 2018

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