Fibhash trick #123
Closed
Fibhash trick #123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SkBlaz
requested review from
yonatankarni,
bbenshalom,
ggaspersic and
adischw
bbenshalom
requested changes
Nov 14, 2023
| @@ -334,6 +335,21 @@ impl FeatureBufferTranslator { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // rehash part | |||
There was a problem hiding this comment.
Let's change this comment to explain that this is an implementation of fibhash and a add link to documentation
| @@ -334,6 +335,21 @@ impl FeatureBufferTranslator { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // rehash part | |||
| for fb_el in self.feature_buffer.ffm_buffer.iter_mut() { | |||
There was a problem hiding this comment.
this and the next loop can be merged into one
There was a problem hiding this comment.
Lines 342 & 349 are different. If you can zip it with those 2 values, you can ignore the repeating if check you can indeed merge them. Although an if here shouldn't have that large of an impact
yonatankarni
reviewed
Dec 6, 2023
|
yes, and does bot offer consistently better results, so not merging
…On Wed, 6 Dec 2023, 08:26 Yonatan Karni, ***@***.***> wrote:
***@***.**** commented on this pull request.
I don't understand - this comes in addition to the FNV hash, it doesn't
replace it?
—
Reply to this email directly, view it on GitHub
<#123 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACMSERBJZKQUUZFTSBV3CL3YIAMZDAVCNFSM6AAAAAA7I543GKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONRWHAZTCMRXG4>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
|
but did you try using Fibonacci hashing instead of FNV hash?
…On Wed, Dec 6, 2023 at 10:15 AM SkBlaz ***@***.***> wrote:
yes, and does bot offer consistently better results, so not merging
On Wed, 6 Dec 2023, 08:26 Yonatan Karni, ***@***.***> wrote:
> ***@***.**** commented on this pull request.
>
> I don't understand - this comes in addition to the FNV hash, it doesn't
> replace it?
>
> —
> Reply to this email directly, view it on GitHub
> <
#123 (review)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/ACMSERBJZKQUUZFTSBV3CL3YIAMZDAVCNFSM6AAAAAA7I543GKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONRWHAZTCMRXG4>
> .
> You are receiving this because you modified the open/close state.Message
> ID: ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#123 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZCJMI5ROD7AO6DPPZJ6OTYIASRHAVCNFSM6AAAAAA7I543GKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBSGM4DOMJVGI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
--
The above terms reflect a potential business arrangement, are provided
solely as a basis for further discussion, and are not intended to be and do
not constitute a legally binding obligation. No legally binding obligations
will be created, implied, or inferred until an agreement in final form is
executed in writing by all parties involved.
This email and any
attachments hereto may be confidential or privileged. If you received this
communication by mistake, please don't forward it to anyone else, please
erase all copies and attachments, and please let me know that it has gone
to the wrong person. Thanks.
|
|
No, it's a replacement for the mod's biases, not whole hash. Alternative
would be mm3 + this directly, that wasn't tested
…On Wed, 6 Dec 2023, 09:44 Yonatan Karni, ***@***.***> wrote:
but did you try using Fibonacci hashing instead of FNV hash?
On Wed, Dec 6, 2023 at 10:15 AM SkBlaz ***@***.***> wrote:
> yes, and does bot offer consistently better results, so not merging
>
> On Wed, 6 Dec 2023, 08:26 Yonatan Karni, ***@***.***> wrote:
>
> > ***@***.**** commented on this pull request.
> >
> > I don't understand - this comes in addition to the FNV hash, it
doesn't
> > replace it?
> >
> > —
> > Reply to this email directly, view it on GitHub
> > <
>
#123 (review)>,
>
> > or unsubscribe
> > <
>
https://github.com/notifications/unsubscribe-auth/ACMSERBJZKQUUZFTSBV3CL3YIAMZDAVCNFSM6AAAAAA7I543GKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONRWHAZTCMRXG4>
>
> > .
> > You are receiving this because you modified the open/close
state.Message
> > ID: ***@***.***>
> >
>
> —
> Reply to this email directly, view it on GitHub
> <
#123 (comment)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AAZCJMI5ROD7AO6DPPZJ6OTYIASRHAVCNFSM6AAAAAA7I543GKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBSGM4DOMJVGI>
> .
> You are receiving this because your review was requested.Message ID:
> ***@***.***>
>
--
The above terms reflect a potential business arrangement, are provided
solely as a basis for further discussion, and are not intended to be and
do
not constitute a legally binding obligation. No legally binding
obligations
will be created, implied, or inferred until an agreement in final form is
executed in writing by all parties involved.
This email and any
attachments hereto may be confidential or privileged. If you received
this
communication by mistake, please don't forward it to anyone else, please
erase all copies and attachments, and please let me know that it has gone
to the wrong person. Thanks.
—
Reply to this email directly, view it on GitHub
<#123 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACMSERGX4FL7U4BGJXOMZNDYIAV5HAVCNFSM6AAAAAA7I543GKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBSGQZTQMZWGQ>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
|
yes, I don't mean a replacement for mmh3 - but for feature combinations we
use FNV hash so we could replace that as well.
but it's probably negligible compared to mmh3 anyway...
…On Wed, Dec 6, 2023 at 11:57 AM SkBlaz ***@***.***> wrote:
No, it's a replacement for the mod's biases, not whole hash. Alternative
would be mm3 + this directly, that wasn't tested
On Wed, 6 Dec 2023, 09:44 Yonatan Karni, ***@***.***> wrote:
> but did you try using Fibonacci hashing instead of FNV hash?
>
> On Wed, Dec 6, 2023 at 10:15 AM SkBlaz ***@***.***> wrote:
>
> > yes, and does bot offer consistently better results, so not merging
> >
> > On Wed, 6 Dec 2023, 08:26 Yonatan Karni, ***@***.***> wrote:
> >
> > > ***@***.**** commented on this pull request.
> > >
> > > I don't understand - this comes in addition to the FNV hash, it
> doesn't
> > > replace it?
> > >
> > > —
> > > Reply to this email directly, view it on GitHub
> > > <
> >
>
#123 (review)>,
>
> >
> > > or unsubscribe
> > > <
> >
>
https://github.com/notifications/unsubscribe-auth/ACMSERBJZKQUUZFTSBV3CL3YIAMZDAVCNFSM6AAAAAA7I543GKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONRWHAZTCMRXG4>
>
> >
> > > .
> > > You are receiving this because you modified the open/close
> state.Message
> > > ID: ***@***.***>
> > >
> >
> > —
> > Reply to this email directly, view it on GitHub
> > <
>
#123 (comment)>,
>
> > or unsubscribe
> > <
>
https://github.com/notifications/unsubscribe-auth/AAZCJMI5ROD7AO6DPPZJ6OTYIASRHAVCNFSM6AAAAAA7I543GKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBSGM4DOMJVGI>
>
> > .
> > You are receiving this because your review was requested.Message ID:
> > ***@***.***>
> >
>
> --
> The above terms reflect a potential business arrangement, are provided
> solely as a basis for further discussion, and are not intended to be and
> do
> not constitute a legally binding obligation. No legally binding
> obligations
> will be created, implied, or inferred until an agreement in final form
is
> executed in writing by all parties involved.
>
>
> This email and any
> attachments hereto may be confidential or privileged. If you received
> this
> communication by mistake, please don't forward it to anyone else, please
> erase all copies and attachments, and please let me know that it has
gone
> to the wrong person. Thanks.
>
> —
> Reply to this email directly, view it on GitHub
> <
#123 (comment)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/ACMSERGX4FL7U4BGJXOMZNDYIAV5HAVCNFSM6AAAAAA7I543GKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBSGQZTQMZWGQ>
> .
> You are receiving this because you modified the open/close state.Message
> ID: ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#123 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZCJMLQPBNKFJGGPDBWELTYIA6RBAVCNFSM6AAAAAA7I543GKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBSGU2TGMZWGI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
--
The above terms reflect a potential business arrangement, are provided
solely as a basis for further discussion, and are not intended to be and do
not constitute a legally binding obligation. No legally binding obligations
will be created, implied, or inferred until an agreement in final form is
executed in writing by all parties involved.
This email and any
attachments hereto may be confidential or privileged. If you received this
communication by mistake, please don't forward it to anyone else, please
erase all copies and attachments, and please let me know that it has gone
to the wrong person. Thanks.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Seems to work, ripe for first reviews. Rationale here. Ignoring some tests for now fyi