remove seed variable in BloomFilter and rename k to hashIndex #557
Conversation
sritchie
force-pushed
the
sritchie/remove_seed
branch
from
715faa1
to
c0b7739
Compare
|
(fails binary compatibility check, of course) |
|
I think this is a good PR. we accepted this implementation as a contribution and didn't look back too much (or at least accepted a revamp of it). This looks like a good improvement. I don't know why you need a seed, except to perhaps salt the hash function, for a reason I don't know. Maybe to allow multiple independent blooms, but why not just use one bigger one there, I'm sure it would perform better? In any case. I would say merge this on the next binary break (which will be when we merge #523 |
| @@ -361,19 +361,19 @@ case class BFHash(numHashes: Int, width: Int, seed: Long = 0L) extends Function1 | |||
| (upper, lower) | |||
| } | |||
|
|
|||
| private def nextHash(bytes: Array[Byte], k: Int, digested: Seq[Int] = Seq.empty): Stream[Int] = { | |||
| if (k == 0) | |||
| private def nextHash(bytes: Array[Byte], hashIndex: Int, digested: Seq[Int] = Seq.empty): Stream[Int] = { | |||
There was a problem hiding this comment.
we should optimize this. This is terrible using Stream here.
Paging @non
|
looks like we need to remove mima for the tests. |
|
What gets used instead of the passed in seed? A fixed default one? Only thing I can think of is unit tests wanting a fixed seed or something? |
|
We never used the seed at all.
…On Fri, Dec 2, 2016 at 14:23 Alex Levenson ***@***.***> wrote:
What gets used instead of the passed in seed? A fixed default one? Only
thing I can think of is unit tests wanting a fixed seed or something?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#557 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAEJdlIvuRP5NhTomikEyGrJuYg6q4Lnks5rELZpgaJpZM4Ke2Ln>
.
|
|
Oh I see, there's no randomness involved in the first place right? |
|
@isnotinvain yup, that's correct |
|
Nice, This failed because the doc compilation failed! |
|
Sweet!
…On Fri, Dec 2, 2016 at 16:04 Sam Ritchie ***@***.***> wrote:
Nice, This failed because the doc compilation failed!
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#557 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAEJdnqge013QbPetk9M0APfRIiQ0TqKks5rEM4qgaJpZM4Ke2Ln>
.
|
|
👍 |
Same as #220!
I'm GUESSING this might not be legit - do we want to nuke the seed, or do we want to actually try and use it?