mlpack / benchmarks

Machine Learning Benchmark Scripts
101 stars 49 forks source link

Implementation of Approximate Nearest Neighbours using the Nearpy Library. #52

Closed Iron-Stark closed 7 years ago

Iron-Stark commented 7 years ago

@zoq @rcurtin

Implemented the Approximate Nearest Neighbors algorithm using the Nearpy Library. Also wrote the benchmark test file for the same. Please Review.

mlpack-jenkins commented 7 years ago

Can one of the admins verify this patch?

rcurtin commented 7 years ago

We'll have to also include a script to install nearpy; could you add that please? Just like you did for annoy.

Iron-Stark commented 7 years ago

@rcurtin

I have added installation files. Please Review.

rcurtin commented 7 years ago

Before merging this, I need to set up the automatic installs for libraries on Jenkins, so that nearpy can be automatically installed, but today I have to finish making a poster for a conference. I'll hopefully be able to address that this week.

Iron-Stark commented 7 years ago

@rcurtin

Thanks for the reply. :-)

rcurtin commented 7 years ago

Looks good to me other than @zoq's comments---if you can handle those I think this is ready to merge. The testing infrastructure is set up right on Jenkins now so it is much easier to add new libraries like this one. :)

Iron-Stark commented 7 years ago

@zoq @rcurtin I have made the suggested changes. Please Review.

Iron-Stark commented 7 years ago

@mlpack-jenkins test this

zoq commented 7 years ago

The jenkins plugin is really picky about the trigger message and the format, meaning you have to write the message in a single line.

Iron-Stark commented 7 years ago

@zoq Okay will remember that from now on. Thanks for the info. :-)

Iron-Stark commented 7 years ago

@mlpack-jenkins test this

Iron-Stark commented 7 years ago

@zoq @rcurtin

javac: invalid flag: benchmark/libraries/share//weka.jar This is the error popping up during build. Not able to figure out a concrete reason. Seems to me that the '//' caused the error. Please suggest how to fix it.

zoq commented 7 years ago

There was an issue where a directory with spaces wasn't handled correctly, it's fixed in: 28cdb6910fddf8fa29d1a277176573cd5bb3563b

zoq commented 7 years ago

@mlpack-jenkins test this

zoq commented 7 years ago

Also, if I didn't missed something, the Jenkins job should only rebuild all libraries if something in the libraries folder changed, so it should run much faster if we only change something in methods, tests, etc. But I guess, we will see if that works if we open another PR that doesn't introduce a new library.

zoq commented 7 years ago

@mlpack-jenkins test this

zoq commented 7 years ago

Looks like chmod 755 milk_install.shwould solve the remaining issue.

Iron-Stark commented 7 years ago

@zoq @rcurtin

Looks like there was some error installing nearpy. Any suggestions on how to avoid that?

Iron-Stark commented 7 years ago

I guess its because the filename is NearPy and not nearpy. Should I change it to that and try again?

zoq commented 7 years ago

chmod 755 nearpy.sh should solve the problem.

Iron-Stark commented 7 years ago

@zoq

In which file should I add the chmod 755. In the install_all.sh file or nearpy_install.sh file? and where? A bit confused regarding this.

zoq commented 7 years ago

Let me clarify what I mean with chmod 755 nearpy.sh, if you take a look at the permissions of nearpy_intall.sh e.g. ls -l nearpy_install.sh you should get something like: -rw-r--r-- ... which tells you that:

rw-
User can read (r)
User can write (w)
User cannot execute (x)

r--
Group can read (r)
Group cannot write (w)
Group cannot execute (x)

r--
Other can read (r)
Other cannot write (w)
Other cannot execute (x)

but what we like is that jenkins the user can execute the script:

-rwx... e.g. 755 - I/Jenkins can read/write and run it, everyone else can run it.

rwx
User can read (r)
User can write (w)
User can execute (x)

So, basically all you have to do is to change the permissions of the nearpy install script and push the changes:

chmod 755 nearpy_install.sh
git add nearpy_install.sh
git commit -m "Make Nearpy install script executable."
git push

Let me know if that is helpful.

Iron-Stark commented 7 years ago

@zoq Thanks for clearing that out.

zoq commented 7 years ago

Thanks for the great contribution.