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

Added binding for MurmurHash2 #24

Merged
merged 1 commit into from
Dec 12, 2021
Merged

Conversation

JohanMabille
Copy link
Contributor

@JohanMabille JohanMabille commented Feb 16, 2021

Hi,

This adds cython bindings for MurmurHash2 method. We use the native implementation in xeus and a webasm implementation in jupyterlab. We would need the python bindings for ipykernel.

@honnibal
Copy link
Member

honnibal commented Feb 17, 2021

Sure, happy to have this. I think you need to add the function though.

The way I have this set up is that the extern is done in the .pyx file, not in the .pxd. This prevents the user code from needing to find the header file. But the consequence is that you won't be able to import the function you just exposed --- it needs to have a Cython function, analogous to the others that are there, and that function needs to be exposed in the .pxd

If the above doesn't make sense to you (I get that Cython's a big language with many mechanisms), I'm happy to make the change.

@JohanMabille
Copy link
Contributor Author

JohanMabille commented Feb 17, 2021

Ah indeed, sorry for that, I'm new to Cython. I've plugged the MurmurHash2 method into the existing methods to avoid duplicating existing code, and I added a version argument to choose between the different versions. A default value is provided so that it is backward compatible, let me know if you prefer a dedicated bunch of functions for MurmurHash2 instead.

@JohanMabille JohanMabille force-pushed the murmurhash2 branch 2 times, most recently from 2af46d8 to ed20038 Compare February 17, 2021 16:29
@SylvainCorlay
Copy link

Ping @honnibal
It would be super great if this was merged!

@honnibal honnibal merged commit d086eff into explosion:master Dec 12, 2021
@JohanMabille JohanMabille deleted the murmurhash2 branch December 13, 2021 09:05
adrianeboyd added a commit to adrianeboyd/murmurhash that referenced this pull request Dec 15, 2021
adrianeboyd added a commit that referenced this pull request Dec 15, 2021
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

Successfully merging this pull request may close these issues.

3 participants