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

Non-blocking periodic_events method on DynamicMap #1429

Merged
merged 18 commits into from May 16, 2017
Merged

Conversation

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented May 10, 2017

This PR aims to address #1428 and replaces the Periodic stream prototype we were testing. It should be possible for the bokeh renderer to install a bokeh appropriate utility when running as a bokeh app.

Action items

  • Add general periodic utility to core (simple threaded approach)
  • Add periodic_events method to DynamicMap.
  • All count of None to run indefinitely
  • Bokeh server support (i.e when running as a bokeh app)
  • Update tutorials to mention this and use it
  • Unit tests
@jlstevens jlstevens added this to the v1.7.1 milestone May 10, 2017
@jlstevens
Copy link
Contributor Author

@jlstevens jlstevens commented May 10, 2017

@philippjfr

I don't have anything more to do on this PR right now. If you could figure out how to get this working when running as a bokeh app, that would be great!

I won't be touching this again (until you tell me to) so don't worry about merge conflicts and just push your changes here. Meanwhile I'll prepare some unit tests for the default util but I won't commit that yet.

Loading

@jlstevens
Copy link
Contributor Author

@jlstevens jlstevens commented May 10, 2017

Maybe a stop() method on DynamicMap would be nice instead of having to grab a handle on something that has stop(). DynamicMap would have to hold onto these thread like objects internally so it could stop all instances that might still be running.

Loading

@jlstevens
Copy link
Contributor Author

@jlstevens jlstevens commented May 14, 2017

In the last commit, I implemented a slightly different approach - a periodic object with a stop method. This means you can do something lik this:

dmap.periodic(0.001, 1000000)
dmap.periodic.stop() # To stop whenever you want 

In addition to making stopping periodic callbacks more convenient, this approach allows _periodic_util attribute to be set on the periodic class. This can be set to something different from the default e.g in a bokeh app context. In addition the instances of this utility are available via the instances attribute which, by default, is used to stop anything that is running but can also be accessed by the renderer if necessary.

@philippjfr I think this approach should fit better with the bokeh app support. Over to you!

Loading

@jlstevens
Copy link
Contributor Author

@jlstevens jlstevens commented May 14, 2017

I've just had another thought: maybe it would be conceptually simpler to only have one instance.

If we did that, then when you run __call__, it stops that instance (if it is already running). As the event method can update all the stream arguments at once, I'm not sure why we would want to have multiple periodic runner instances.

Loading

if isinstance(o, DynamicMap):
layer_streams += get_nested_streams(o)
return list(set(layer_streams))
return list({s for dmap in get_nested_dmaps(dmap) for s in dmap.streams})
Copy link
Contributor Author

@jlstevens jlstevens May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to refactor it this way.

Loading

else:
plot.document = doc
plot = plot.plot
plot.document = doc
Copy link
Contributor Author

@jlstevens jlstevens May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment as a reminder of what attach_periodic does here...

Loading

@classmethod
def is_set(cls):
return self.count == 0
return completed
Copy link
Contributor Author

@jlstevens jlstevens May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the API of the other periodic utility so it doesn't need to be this complex. I'll also simplify this.

Loading

@jlstevens
Copy link
Contributor Author

@jlstevens jlstevens commented May 15, 2017

@philippjfr All looks good!

I'll update the stuff around completed and is_set to simplify it, but otherwise we just need to add unit tests and update the docs. Maybe one of the existing bokeh app examples should use it...if not maybe add another one?

Any way you can set up bokeh specific unit tests for that utility? I'll add some unit tests for the core utility...

Loading


def __str__(self):
return "<holoviews.core.spaces.periodic method>"

Copy link
Member

@jbednar jbednar May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment explaining what this representation is meant to achieve?

Loading

Copy link
Contributor Author

@jlstevens jlstevens May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have another idea of what the repr should be, I would be happy to implement it! What sort of comment are you thinking of?

Loading

Copy link
Member

@jbednar jbednar May 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just something to indicate why the default representation was not suitable. I.e. why was this method necessary?

Loading

Copy link
Contributor Author

@jlstevens jlstevens May 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't strictly necessary, but I think it is a bit nicer than the default which is something like:

<holoviews.core.spaces.periodic at 0x10886e850>

Instead of a random memory address, I say that it is a method which reflects how this utility is used (i.e it lives on DynamicMap).

Happy to remove it if you think the default is better.

Loading

Copy link
Member

@jbednar jbednar May 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to remove it, just put a docstring saying what you say here (that this repr provides additional information that should be more useful).

Loading

@jlstevens
Copy link
Contributor Author

@jlstevens jlstevens commented May 16, 2017

@philippjfr Ready for review once the tests go green.

Loading

@jlstevens jlstevens force-pushed the periodic_utility branch from fe5c324 to fa983f6 May 16, 2017
@jlstevens
Copy link
Contributor Author

@jlstevens jlstevens commented May 16, 2017

@philippjfr The pr tests have passed and I don't think the push build ones will unless I rebase.

Loading

@philippjfr
Copy link
Member

@philippjfr philippjfr commented May 16, 2017

Looks good, merging.

Loading

@philippjfr philippjfr merged commit 8b68e26 into master May 16, 2017
3 of 4 checks passed
Loading
@philippjfr philippjfr deleted the periodic_utility branch May 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants