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

SampleWithObservable broken? #912

Closed
dvtomas opened this issue Feb 20, 2014 · 12 comments
Closed

SampleWithObservable broken? #912

dvtomas opened this issue Feb 20, 2014 · 12 comments

Comments

@dvtomas
Copy link

dvtomas commented Feb 20, 2014

Hi all,
I'd expect

import rx.lang.scala.Observable
import scala.concurrent.duration._
import rx.lang.scala.JavaConversions._

val o = Observable.items(1) ++ Observable.never
val sampler = Observable.interval(100.milli).take(3)
toScalaObservable(toJavaObservable(o) sample toJavaObservable(sampler)).toBlockingObservable.toList

to return List(1, 1, 1), but it returns (as of As of RxJava 0.16.1) only List(1). Furthermore, it seems that there is no unit test in the source testing SampleWithObservable.

Thank you for any insights on this. Best regards,
Tomáš Dvořák

@dvtomas
Copy link
Author

dvtomas commented Feb 21, 2014

I just found OperationSampleTest.java, so there are in fact unit tests, sorry for not finding them earlier. There is even a test sampleWithSamplerNoDuplicates, so it seems that it is actually in the specification of sample to ignore the same value if sampled more than one times. I was mislead by the marble diagram at http://netflix.github.io/RxJava/javadoc/rx/Observable.html#sample(rx.Observable)
which shows that the green circle gets emitted two times when sampled twice. The marble diagram behavior is also the one I'd prefer (you can always filter the duplicate values via distinct, but you can't do the reverse). So, is there any chance the behavior of sample could be changed, or maybe could we get another operator that wouldn't discard the duplicate values?

Thank you, regards,
Tomáš Dvořák

@DavidMGross
Copy link
Collaborator

Tomáš -

Did you get a definitive answer about this? I'm the fellah who put the
diagram together based on my understanding of sample() at the time. If I've
got it wrong, I'd like to change it to match the actual behavior.

-- David

On Fri, Feb 21, 2014 at 6:52 AM, Tomas Dvorak notifications@github.comwrote:

I just found OperationSampleTest.java, so there are in fact unit tests,
sorry for not finding them earlier. There is even a test
sampleWithSamplerNoDuplicates, so it seems that it is actually in the
specification of sample to ignore the same value if sampled more than one
times. I was mislead by the marble diagram at
http://netflix.github.io/RxJava/javadoc/rx/Observable.html#sample(rx.Observable)
which shows that the green circle gets emitted two times when sampled
twice. The marble diagram behavior is also the one I'd prefer (you can
always filter the duplicate values via distinct, but you can't do the
reverse). So, is there any chance the behavior of sample could be changed,
or maybe could we get another operator that wouldn't discard the duplicate
values?

Thank you, regards,
Tomáš Dvořák


Reply to this email directly or view it on GitHubhttps://github.com//issues/912#issuecomment-35736613
.

David M. Gross
PLP Consulting

@dvtomas
Copy link
Author

dvtomas commented Feb 27, 2014

No answer yet, but I hope that it will be the behavior that will get changed to match the marble diagram :) We'll see..

@akarnokd
Copy link
Member

akarnokd commented May 8, 2014

Hi. Both sample with time and sample with observable consume source values only once. Do you still need a sampler which returns the last seen value from source?

@DylanSale
Copy link

I had to write a custom operator to do this (always emit the last value when sampler fires) so I'd appreciate this functionality being in the library.

I'm not in a position to contribute my implementation sorry.

@benjchristensen
Copy link
Member

So would this be an operator that emits the latest value on every tick? If so, it's not really sample so what would that be called?

@dvtomas
Copy link
Author

dvtomas commented May 31, 2014

Hi,
I'm on a long vacation so I'm not able to test anything or respond too
often. But I can't see, how the behavior of emitting the last value on
every tick is not sampling - when you sample e. g. an AD signal with a
sampling tick signal, you still want to sample a value on every tick
regardless of whether the source signal value has changed or not - and that
is how I percieve what sampling means. If you don't want duplicate
values, you can always distinct it.

On Tue, May 20, 2014 at 9:03 AM, Ben Christensen notifications@github.com
wrote:

So would this be an operator that emits the latest value on every tick? If
so, it's not really sample so what would that be called?


Reply to this email directly or view it on GitHub
#912 (comment).

@headinthebox
Copy link
Contributor

Unlike map, filter, etc. that have a fimr formal categorical semantics, what sampling means is purely subjective, so as the API designer you have to pick a few behaviors among many possible ones, with the risk there are scenarios where the built-in ones don't work for some people.

Fortunately, it is typically easy to roll your own, especially with the new lift operator. That's the power of compositionality.

Now in this particular case, doing a distinctUntilChanged will not work, since that does not distinguish between the real values in the stream and the repeated values, i.e. if your stream is [1,1,1,1,1,1,1,1,...) you will always get [1).

However, if you want to repeat values or do something else, you can always do that using xs.map(x -> {.... repeat x at some interval of your choosing ...}).switchOnNext() to fill the gaps and then use the regular sample.

@dvtomas
Copy link
Author

dvtomas commented Jun 1, 2014

OK. Just out of curiosity, can you sketch some simple usage scenario for
me, where the current behavior of sample fits in naturally, so that I
better grok what's it intended for, please? Thank you, best regards..

On Sat, May 31, 2014 at 7:54 PM, headinthebox notifications@github.com
wrote:

Unlike map, filter, etc. that have a fimr formal categorical semantics,
what sampling means is purely subjective, so as the API designer you
have to pick a few behaviors among many possible ones, with the risk there
are scenarios where the built-in ones don't work for some people.

Fortunately, it is typically easy to roll your own, especially with the
new lift operator. That's the power of compositionality.

Now in this particular case, doing a distinctUntilChanged will not work,
since that does not distinguish between the real values in the stream and
the repeated values, i.e. if your stream is [1,1,1,1,1,1,1,1,...) you
will always get [1).

However, if you want to repeat values or do something else, you can always
do that using xs.map(x -> {.... repeat x at some interval of your
choosing ...}).switchOnNext() to fill the gaps and then use the regular
sample.


Reply to this email directly or view it on GitHub
#912 (comment).

@dvtomas
Copy link
Author

dvtomas commented Jun 16, 2014

Hmm. I have checked all three examples, but I'm still not convinced, that any of them requires precisely the behavior that is implemented by the current version of sample, that is, sample values, but don't emit the value on next sampler tick if a new source value hasn't been emitted. The first example doesn't have any specification, the second one should be in my opinion better with my definition of sample followed by distinct, and the same could arguably be said about the third example.

I have also noticed that the author of the the third example mentions that "My intuition was that if underlying feed does not produce a value during the sampling interval, “Sample” will repeat the most recent available value when sampling is due", that is, he was initially under the same impression as me. @DylanSale has posted a comment to this discussion stating that he also had the need the behavior I was describing, and had to implement it himself. The whole issue #1045 seems to be dealing with this once again, where, independently of me, @amazari also desires the behavior I have described. And I still have not found any use case requiring sample to behave exactly the way it behaves now (but my imagination may be limited by the domain of the problems I usually deal with in my job).

However, after this many comments on the topic and no sign of having convinced anybody, I feel like I'm just nitpicking too much, and I certainly don't want to do that. I really do appreciate all the work you put into RxJava and the open and collaborative way of development. @headinthebox has already posted a way to combine operators to achieve the desired behavior (thank you!), and if sample as is now is going to stay, I'm fine with just using his solution and closing this issue.

Best regards,
Tomáš Dvořák

@headinthebox
Copy link
Contributor

@dvtomas, the power of a library is not in how many special-purpose operators it has that do exactly the job you want (i.e. how long the autocomplete list is when you hit .), but how flexible it is to compose the solution you want using an orthogonal set of primitive operators (i.e. how short the autocomplete list is when you hit .), i.e. when it is possible to "combine operators to achieve the desired behavior", then the bar to add that combination as a separate operator is super high.

It seems you are using Scala, so there it is really easy to add this operator yourself as an extension method and it will look as if it is a built-in operator. Unfortunately for Java we don't have that choice, so we need to be extra strict to keep the number of operators down and avoid to get an obese API.

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

No branches or pull requests

6 participants