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

HoloMap and default params #704

Closed
vascotenner opened this issue Jun 3, 2016 · 17 comments
Closed

HoloMap and default params #704

vascotenner opened this issue Jun 3, 2016 · 17 comments
Assignees
Labels
tag: API type: feature A major new feature
Milestone

Comments

@vascotenner
Copy link
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
Copy link
Member

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.

@philippjfr philippjfr added type: feature A major new feature tag: API labels Jun 17, 2016
@jbednar
Copy link
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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

... 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
Copy link
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
Copy link
Contributor

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
Copy link
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 modified the milestones: v1.7.1, v1.8 Apr 27, 2017
@philippjfr philippjfr modified the milestones: v1.9, v1.8 Jun 15, 2017
@jordansamuels
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Member

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
Copy link

This issue 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 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tag: API type: feature A major new feature
Projects
None yet
Development

No branches or pull requests

5 participants