WIP: add sample stage #23
Conversation
| completeStage() | ||
| } | ||
|
|
||
| @throws[Exception](classOf[Exception]) |
There was a problem hiding this comment.
these throws should not be needed, since this is scala
|
looking good, @zhxiaogg |
|
|
||
| @throws[Exception](classOf[Exception]) | ||
| override def onUpstreamFinish(): Unit = { | ||
| completeStage() |
There was a problem hiding this comment.
this is the default anyway, no need for the override.
|
Small improvements needed, otherwise looks good. |
zhxiaogg
force-pushed
the
feature/add_sample_stage
branch
from
6120ca2
to
ea0fa9f
Compare
|
LGTM |
| */ | ||
| def random[T](maxStep: Int = 1000): Sample[T] = { | ||
| require(maxStep > 0, "max step for a random sampling must > 0") | ||
| Sample[T](() => ThreadLocalRandom.current().nextInt(maxStep) + 1) |
There was a problem hiding this comment.
Unfortunately the numbers returned by this call are super-bad quality from a scientific point of view. I think this should be detailed in the scaladoc and recommend people that need really good behaved sampling to use their own (XorShift family for example) explicitly.
There was a problem hiding this comment.
Also, this does not provide truly independent sampling either, so maybe call this randomStep instead of random.
|
I don't want to prolong this review forever, but I feel an interface |
|
@2m sure, no problem. @drewhk I think the current interface of Sample case class is enough for people to inject their own sampling logic (eg. XorShift). |
|
Ok, thanks for explaining. LGTM |
| @@ -0,0 +1,74 @@ | |||
| package akka.stream.contrib | |||
There was a problem hiding this comment.
We don't do license headers in this repo?
Thought we should
There was a problem hiding this comment.
We do. Missing header here was an indication that sbt did not see that file. And that was because it was not moved to the contrib dir. I created a PR to fix it: #55
|
It's useful enough, thanks @zhxiaogg - LGTM |
ref #21
I started by a simple implementation.
questions:
def apply(maxStep:Int, random:Random=Random)for random sampling, but looks wired. Any suggestion?Feel free to review or close it.