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

Explore @lemire's rolling hash function as an alternative for Counttable #1616

Closed
ctb opened this issue Feb 12, 2017 · 18 comments
Closed

Explore @lemire's rolling hash function as an alternative for Counttable #1616

ctb opened this issue Feb 12, 2017 · 18 comments

Comments

@ctb
Copy link
Member

ctb commented Feb 12, 2017

See https://github.com/lemire/rollinghashcpp/blob/master/example.cpp

Should be much faster than murmurhash.

@betatim
Copy link
Member

betatim commented Feb 14, 2017

Once #1511 is merged this is going to be easy :)

As a first step I would take the code from lemire/rollinghashcpp, put what we need in third-party/ and add a KarpRabinKmerHashIterator

@asl
Copy link

asl commented May 9, 2017

Note that the version above only supports 19 bit hashes. Something like https://github.com/bcgsc/ntHash is more universal, however, it's under GPLv3

@lemire
Copy link

lemire commented May 18, 2017

@asl

Note that the version above only supports 19 bit hashes.

19 is the default, not the sole setting.

19 is an odd default, I'll grant you that.

@asl
Copy link

asl commented May 18, 2017

@lemire

Probably I was confused by the following code:

if(wordsize == 19) {
            irreduciblepoly = 1 + (1<<1) + (1<<2) + (1<<5)
                               + (1<<19);
        } else if (wordsize == 9) {
            irreduciblepoly = 1+(1<<2)+(1<<3)+(1<<5)+(1<<9);
        } else {
            cerr << "unsupported wordsize "<<wordsize << " bits, try 19 or 9"<< endl;
        }

@lemire
Copy link

lemire commented May 18, 2017

@asl Right. One of our templates has weird limitations, but that's not the one you are thinking about using, I believe.

@sjackman
Copy link

sjackman commented Aug 16, 2017

@asl
Copy link

asl commented Aug 16, 2017

@sjackman Unfortunately, it's GPL3. And I already mentioned it above :) Anyway, #1699 contains standalone rolling hash implementation extracted from the code of @lemire

@sjackman
Copy link

Ah, woops. My mistake.
@mohamadi How do you feel about dual licensing ntHash GPL-3 and MIT?

@mohamadi
Copy link

@sjackman @asl Sure. I'll make it dual licensing now.

@sjackman
Copy link

Sounds like it's a moot point for khmer, since you already have #1699. If the license prevented khmer from adopting ntHash, it could be a similar impediment to other projects. Dual licensing could help its uptake.

@sjackman
Copy link

Awesome. Good show, Hamid.
I'd suggest changing the LICENSE file to the more permissive MIT license, and mention in the README.md that it's dual licensed MIT or GPL-3.

@sjackman
Copy link

@asl If the license is the only thing holding you back from using a project, it never hurts to ask the author to consider relicensing.

@sjackman
Copy link

sjackman commented Aug 16, 2017

On second thought, there's not much benefit to dual licensing, and it would add confusion. May as well just go MIT license only.

@mohamadi
Copy link

I just changed the license from GPL3 to only MIT.

@sjackman
Copy link

Thanks, Hamid!

@standage
Copy link
Member

Great. Thanks for the responses!

@sjackman
Copy link

Happy to!

@ctb
Copy link
Member Author

ctb commented Nov 4, 2017

I think this is fixed by #1792!

@ctb ctb closed this as completed Nov 4, 2017
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

7 participants