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

Supporting streams dictionaries in operations #4818

Merged
merged 18 commits into from Feb 2, 2021

Conversation

jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Feb 1, 2021

WIP PR to address #4807.

For testing, I've defined the streams parameter of inspect_points as:

    streams = param.ClassSelector(default=dict(x=PointerXY.param.x,
                                               y=PointerXY.param.y),
                                  class_=(dict, list))

And here is the corresponding, working test example:

PR

However, there are some serious limitations right now. The minor one is that I have to set hv.streams.PointerXY.x = 0 and
hv.streams.PointerXY.y = 0 due to an error processing dictionaries when allowing the None default. While I'm sure this can be fixed, this seems to be a far more fundamental problem right now:

As operations use stream classes, not instances, how can I get back to the correct class with something like Tap.param.x when the owner of the parameter may be a superclass? The code needs to instantiate the correct stream instance. Any ideas @jbednar @philippjfr ?

If this can be solved quickly then I can work on this PR to get it into the next minor release. However, if this is a deeper issue (as I suspect it is) then this PR will need to wait for updates in param at the very least. Assuming this has a quick solution, the next steps would be:

  • Fix the dictionary keyword issue when the parameter values are initialized with None.
  • See if this dictionary support can be pushed further down into Apply and/or Dynamic.

@jlstevens
Copy link
Contributor Author

jlstevens commented Feb 1, 2021

One possible solution to the owner problem is e75743f: simply make sure all parameters exist on each stream class without relying on inheritance.

The logic is that 1) this mechanism is primarily about linked streams, you may as well use parameters directly for the old non-linked case 2) linked streams have to be tied to the plotting backend which means they are unlikely to be user defined (and if they are, users would just need to know that they shouldn't rely on parameter inheritance) and 3) we can do something like the following to enforce that all the streams we provide are defined according to the rule:

objs = [getattr(hv.streams, el) for el in dir(hv.streams)]
stream_classes = [obj for obj in objs if isinstance(obj, type) and  issubclass(obj, hv.streams.Stream) ]
for stream_class in stream_classes:
    for name, p in stream_class.param.params().items():
        if name != 'name':
           if p.owner != stream_class:
              raise AssertionError('Parameter %s has owner %s when accessed from class %s' 
                                                      % (name, p.owner, stream_class))

Edit: Or better, I can use stream_classes = param.concrete_descendents(hv.streams.Stream). Should I test all streams or only linked streams?

@jbednar
Copy link
Member

jbednar commented Feb 1, 2021

Sounds good. Seems like only linked streams need testing like that at this time.

@jlstevens
Copy link
Contributor Author

jlstevens commented Feb 1, 2021

@philippjfr I've pushed the changes we discussed in e71782f. Please do push better fixes to how the Params stream is handled!

Co-authored-by: Philipp Rudiger <prudiger@anaconda.com>
holoviews/streams.py Outdated Show resolved Hide resolved
@philippjfr
Copy link
Member

Also happy with this now, just needs a few unit tests.

@jlstevens
Copy link
Contributor Author

Added some unit tests which are now passing. @philippjfr happy to merge?

@jlstevens
Copy link
Contributor Author

@philippjfr Addressed your latest suggestions and the tests are green again. Merge?

@philippjfr philippjfr merged commit 4dbdb84 into master Feb 2, 2021
@philippjfr philippjfr deleted the operations_streams_dict branch April 25, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants