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

Initial prototype of triggering with stream parameter hooks #1740

Closed
wants to merge 1 commit into from

Conversation

jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Jul 20, 2017

This is a prototype of an idea I had during the SciPy 2017 sprints that would let us relax the requirement for constant=True for stream parameters.

The reason stream parameters should always be declared constant currently is that stream parameter should only be changed via the event and update interfaces to ensure clean semantics regarding event triggering: the expectation is that changes to stream parameters trigger updates to the corresponding visualization.

This PR is linked to PR 178 in param (holoviz/param#178) which allows parameter setting to always call trigger:

image

Note that this is a prototype and I still feel there are some open questions:

  1. How useful is this in practice? To use this I need to keep a handle on my stream instance.
  2. Will opening up a new way of using streams be helpful of cause confusion? What is the advantage of this over using dmap.event? Is this just syntactic sugar and is that a good justification?
  3. This syntax is always unbatched (one trigger per parameter changed).

Having to keep a stream handle is annoying in particular, which is why I prefer dmap.event. What I can imagine doing is making dmap.event into an object with a __call__ method (behaves as normal) but that supports the same setting syntax for all stream parameters on the dynamicmap e.g:

dmap.event(x=0, y=1,custom=4) # Batched but the unbatched equivalents also work:
dmap.event.x = 0
dmap.event.y = 1
dmap.event.custom = 4

The fact that the new syntax is unbatched also worries me. Setting multiple parameter is best done using dmap.event() which batches the changes together. In other words, the proposed syntax is only a good replacement for dmap.event() when setting a single parameter.

@philippjfr
Copy link
Member

Do you want to keep this open or create a new PR when we've figured this out at the param level?

@jlstevens
Copy link
Contributor Author

I don't mind closing this PR but then we risk forgetting about this idea, if we still think it has any value.

@philippjfr
Copy link
Member

I don't mind closing this PR but then we risk forgetting about this idea

I don't think there's much risk of that tbh. I definitely want this but it's quite dependent on changes in param happening. Maybe we should open an issue linking to the relevant discussion in param?

@jlstevens
Copy link
Contributor Author

Maybe we should open an issue linking to the relevant discussion in param?

Yes, that's a good idea. It might also be a good idea if you do it, expressing what you the advantages might be if we implement this concept.

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

Successfully merging this pull request may close these issues.

2 participants