Add new Derived stream class #4532

jonmmease · 2020-07-24T18:44:50Z

This PR introduces a new Derived stream class that generalizes the concept of having a Stream that produces values based on one or more other input streams.

The SelectionExpr stream had done this in a somewhat ad-hoc way, but with this PR SelectionExpr is now a subclass of the new Derived stream.

The existing SelectionExpr tests pass with minimal updates. I also added a new set of tests for Derived streams directly in holoviews/tests/teststreams.py. This is a good place to look at for a simple example of what a Derived stream subclass looks like.

Let me know if you have any questions or concerns about the approach or API. Thanks!

that generalizes the concept of having a stream that produces values based on one or more other streams. Update SelectionExpr stream to be a Derived stream subclass.

jbednar · 2020-07-24T18:56:57Z

holoviews/tests/teststreams.py

-            self.assertEqual(len(expr_stream._source_streams), 2)
-            self.assertIsInstance(expr_stream._source_streams[0], SelectionXY)
-            self.assertIsInstance(expr_stream._source_streams[1], Lasso)
+            self.assertEqual(len(expr_stream.input_streams), 2)


Is there a particular reason to switch from source to input here? Does that help make the terminology more consistent?

Here was my thinking. For the SelectionExpr stream, it happened to be the case that all of the input streams were always attached to a single source element. The Derived stream doesn't assume this, all it knows is that it should calculate a value based on a list of streams that come from somewhere else. So I guess it seemed to me that using "source" here was no longer accurate.

Thanks; makes sense to me!

Makes sense to me too.

jonmmease · 2020-07-28T22:05:00Z

The AppVeyor tests are failing due to a bunch a bunch of failures in spatialpandas that I think are pyarrow 1.0 incompatibilities. Though it looks like spatialpandas pins the pyarrow dependency to <1 (https://github.com/holoviz/spatialpandas/blob/master/setup.py#L27), so I'm not sure how this is happening.

Travis tests aren't even being run I don't think:

$ pip install pyctdev && doit miniconda_install && pip uninstall -y doit pyctdev

pyenv: version `3.7' is not installed (set by PYENV_VERSION environment variable)

The command "pip install pyctdev && doit miniconda_install && pip uninstall -y doit pyctdev" failed and exited with 1 during .

Does this mean anything to anyone else?

jbednar · 2020-07-28T22:08:24Z

spatialpandas master pins to <1.0 to avoid those issues, but it hasn't been released yet, so you're presumably still getting an older spatialpandas here. Pinning pyarrow here seems reasonable to me; @jlstevens was looking at releasing spatialpandas.

jbednar · 2020-07-28T22:10:00Z

Oh, and this particular immediate issue is presumably that Travis is providing 3.6 or 3.8, while the rest of the setup is assuming 3.7. That happens every time Travis updates their default images, I think, and we have to adapt. I forget the details, but again @jlstevens will presumably remember.

philippjfr · 2020-07-28T22:10:59Z

The pyenv stuff can be fixed by switching back to language: python in the travis.yml.

philippjfr · 2020-07-28T22:11:54Z

No objection to pinning pyarrow in the test dependencies in setup.py for now.

jonmmease · 2020-07-28T23:06:28Z

I went ahead and got spatialpandas working with pyarrow 1.0 in holoviz/spatialpandas#46. So hopefully this will take care of the issue here once that package is released.

philippjfr · 2020-07-31T14:08:15Z

Looking good, I'll review and hopefully merge this evening.

jonmmease · 2020-08-06T17:08:37Z

Actually, I'm working on the uncollate transform and I may need to revisit this a little. The trigger_index idea doesn't really fit with the stateless goal, so I may need to think on how to remove this.

As you have time, I'd appreciate feedback on #4534 and I'll update this PR once I've gotten further with uncollate. Thanks!

jonmmease · 2020-08-06T18:09:05Z

Ok, I removed trigger_index and added an exclusive option to Derived that, when set, will clear all streams except the most recently updated stream.

jlstevens · 2020-08-07T14:31:01Z

I know this is WIP and I am only raising a naming issue, but I don't think uncollate is a real word. :-)

The antonym I probably liked the most from this list is probably separate but I would like to hear any other suggestions!

jbednar · 2020-08-07T14:44:36Z

It may not be the best word, but it's a word people use: https://www.support.xerox.com/en-us/article/en/x_wc7245_en-O12395

I think it's useful to have the visual semantic link to collate, so that they can be understood as duals. As long as that's true (i.e. that collate(uncollate(x)) == x, at least roughly), a different name would need to be quite compelling before it would unseat uncollate. Conversely, if it's not really the dual of collate, then uncollate is confusing and shouldn't be used...

jonmmease · 2020-08-07T15:23:16Z

I think it's useful to have the visual semantic link to collate, so that they can be understood as duals. As long as that's true (i.e. that collate(uncollate(x)) = x, at least roughly), a different name would need to be quite compelling before it would unseat uncollate

Yeah, I believe these should be very nearly duals. I'll document more when that PR is ready.

@jlstevens, just to clarify, uncollate doesn't show up in this PR. I'll have another PR soon that introduces it. Thanks!

philippjfr · 2020-08-08T21:01:29Z

@jonmmease Could you rebase both PRs? CI is working again on master.

jonmmease · 2020-08-10T09:59:02Z

Updated with master, and things have gotten further!

In AppVeyor, seeing this error

  File "C:\Miniconda36-x64\envs\test\lib\distutils\version.py", line 337, in _cmp
    if self.version < other.version:
TypeError: '<' not supported between instances of 'str' and 'int'

with this in the stack trace:

C:\Miniconda36-x64\envs\test\lib\site-packages\panel\pane\holoviews.py in _render(self, doc, comm, root)
    278 
    279         kwargs = {'margin': self.margin}
--> 280         if backend == 'bokeh' or LooseVersion(str(hv.__version__)) >= str('1.13.0'):
    281             kwargs['doc'] = doc
    282             kwargs['root'] = root
C:\Miniconda36-x64\envs\test\lib\distutils\version.py in __ge__(self, other)

Based on https://bugs.python.org/issue14894, looks like this is an issue with comparing version numbers with different number parts that mix numbers and letters. e.g. this also raises the error LooseVersion("22.7-r1") < LooseVersion("22.7.3-r1").

philippjfr · 2020-08-10T12:39:17Z

Let's fix appveyor separately, as long as Travis passes I'm happy to merge. Only a very minor issue there:

======================================================================
ERROR: holoviews.tests.teststreams.test_all_stream_parameters_constant
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/envs/test-environment/lib/python3.6/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/holoviz/holoviews/holoviews/tests/teststreams.py", line 28, in test_all_stream_parameters_constant
    % (name, stream_cls.__name__))
TypeError: Parameter v of stream Sum not declared constant

jonmmease · 2020-08-10T21:30:16Z

ping @philippjfr. Travis is green now. Thanks for taking the time to look this over!

philippjfr · 2020-08-10T21:31:36Z

Thank you for working on this!

Add new Derived stream class

5efe952

that generalizes the concept of having a stream that produces values based on one or more other streams. Update SelectionExpr stream to be a Derived stream subclass.

jbednar requested review from philippjfr and jlstevens July 24, 2020 18:54

jbednar reviewed Jul 24, 2020

View reviewed changes

jonmmease added 2 commits July 24, 2020 15:22

Fix selection tests

488360e

trigger_index constant

b309f2c

jonmmease mentioned this pull request Jul 29, 2020

Session stateless workflow for HoloViews #4433

Closed

Remove trigger_index, replace with exclusive setting in Derived

d897c2e

Merge remote-tracking branch 'origin/master' into derived_stream

b3c980b

Stream param must be constant

4894351

philippjfr merged commit f075725 into master Aug 10, 2020

jonmmease mentioned this pull request Aug 15, 2020

Add History stream #4554

Merged

philippjfr deleted the derived_stream branch April 25, 2022 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new Derived stream class #4532

Add new Derived stream class #4532

jonmmease commented Jul 24, 2020

jbednar Jul 24, 2020

jonmmease Jul 24, 2020

jbednar Jul 24, 2020

philippjfr Jul 31, 2020

jonmmease commented Jul 28, 2020

jbednar commented Jul 28, 2020

jbednar commented Jul 28, 2020

philippjfr commented Jul 28, 2020

philippjfr commented Jul 28, 2020

jonmmease commented Jul 28, 2020

philippjfr commented Jul 31, 2020

jonmmease commented Aug 6, 2020

jonmmease commented Aug 6, 2020

jlstevens commented Aug 7, 2020

jbednar commented Aug 7, 2020 •

edited

Loading

jonmmease commented Aug 7, 2020

philippjfr commented Aug 8, 2020

jonmmease commented Aug 10, 2020

philippjfr commented Aug 10, 2020

jonmmease commented Aug 10, 2020

philippjfr commented Aug 10, 2020

Add new Derived stream class #4532

Add new Derived stream class #4532

Conversation

jonmmease commented Jul 24, 2020

jbednar Jul 24, 2020

Choose a reason for hiding this comment

jonmmease Jul 24, 2020

Choose a reason for hiding this comment

jbednar Jul 24, 2020

Choose a reason for hiding this comment

philippjfr Jul 31, 2020

Choose a reason for hiding this comment

jonmmease commented Jul 28, 2020

jbednar commented Jul 28, 2020

jbednar commented Jul 28, 2020

philippjfr commented Jul 28, 2020

philippjfr commented Jul 28, 2020

jonmmease commented Jul 28, 2020

philippjfr commented Jul 31, 2020

jonmmease commented Aug 6, 2020

jonmmease commented Aug 6, 2020

jlstevens commented Aug 7, 2020

jbednar commented Aug 7, 2020 • edited Loading

jonmmease commented Aug 7, 2020

philippjfr commented Aug 8, 2020

jonmmease commented Aug 10, 2020

philippjfr commented Aug 10, 2020

jonmmease commented Aug 10, 2020

philippjfr commented Aug 10, 2020

jbednar commented Aug 7, 2020 •

edited

Loading