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

Move MurmurHash3 from utils/ to external/ #468

Closed
ctrl-z-9000-times opened this issue May 13, 2019 · 10 comments
Closed

Move MurmurHash3 from utils/ to external/ #468

ctrl-z-9000-times opened this issue May 13, 2019 · 10 comments
Assignees

Comments

@ctrl-z-9000-times
Copy link
Collaborator

Cleanup from issue #258.
File created in PR #278

MurmurHash3 is used by the random-distributed-scalar-encoder.
This file is not ours, and does not use the same license as nupic.
As such, it should be moved to the "external" directory. The issue
is that the external directory currently is not compiled. Currently it
only contains header files and precompiled libraries, so moving the
new MurMurHash3 file to the external directory will require altering
the CMake files.

@dkeeney
Copy link

dkeeney commented May 13, 2019

This looks like something that should be assigned to me.

@dkeeney dkeeney self-assigned this May 13, 2019
@dkeeney
Copy link

dkeeney commented Jun 12, 2019

I started looking at this issue and found that the MurmurHash3 files have been changed to use namespace nupic and the type UInt32. It includes <nupic/types/Types.hpp>

If I move this to external/common, Types.hpp and the namespace is not available. I would have to remove the namespace and change UInt32 to unsigned int.

Is this what you want?

@ctrl-z-9000-times
Copy link
Collaborator Author

I took another look at this issue, and my description is wrong. The MurmusrHash3 code does not have an incompatible license, it does not have any license! The author wrote:

// MurmurHash3 was written by Austin Appleby, and is placed in the public
// domain. The author hereby disclaims copyright to this source code.

This is free code. Given that no one owns the copyright to this code, we do not need to worry about anyone getting angry about our potential mishandling of the code.

Thanks for looking into this, but I'm going to close this issue.

@breznak
Copy link
Member

breznak commented Jun 12, 2019

The MurmusrHash3 code does not have an incompatible license, it does not have any license! [public domain licensed]

i will reopen this issue, as I think it would be more descriptive to have this under external/. The files there are not due to (in)compatible licences but to designate that we do not develop those files/code (it is third party code which we may update from time to time)

@breznak breznak reopened this Jun 12, 2019
@dkeeney
Copy link

dkeeney commented Jun 12, 2019

Ok, but it contains

#include <nupic/types/Types.hpp>

so that it can use type UInt32. If we move it to external/ we will have to change the use of UInt32 to unsigned int.

@breznak
Copy link
Member

breznak commented Jun 12, 2019

so that it can use type UInt32. If we move it to external/ we will have to change the use of UInt32 to unsigned int.

with a note that those have to match I think that's ok.

Also, dmac modified the file, can you look at the upstream and use that? (ideally, if you again add the cmake downloader, unless that's more work than needed?)

I don't know how active murmur repo is, ideal solution would be a PR there to support templates, and then have us let use it UInt in the needed file (only RDSE?) but that is probably an overkill

@ctrl-z-9000-times
Copy link
Collaborator Author

IIRC The upstream is not active. It is not merging fixes, even ones which pass all of thier unit tests. The maintainer appears to be only interested in the algorithms, and not the implementations.

I modified the upstream code to not use switch case statements with implicit fallthroughs because our build settings disallow that. I also removed all the other functions which we dont need.

@dkeeney
Copy link

dkeeney commented Jun 12, 2019

What you want me to do then is replace UInt32 in our version of MurmurHash3 and move it to external/ and build it there. Add a comment that it must match UInt32.

Ok, I can do that.

@ctrl-z-9000-times
Copy link
Collaborator Author

I still think it would be simpler to just leave it where it is. We could add a few lines of documentation explaining that this isn't our code and where we got it from. But I won't stop you from moving it or changing the data types.

@breznak
Copy link
Member

breznak commented Jun 14, 2019

Closed in #511

@breznak breznak closed this as completed Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants