Closed chogba closed 6 years ago
Can one of the admins verify this patch?
Have added a benchmarking script for shogun's hierarchical clustering implementation along with a testing script to test the benchmarking script
@chogba is there any other lib's hierarchical clustering algorithm present? I don't think so. I mean this is useful to get in, but getting algorithms in which then can be compared to other libs would be even more useful :) Ill make a few comments, leaving the rest to the mlpack crew
@karlnapf yep, makes sense, i'm working on implementing some other shogun models which other libraries have also implemented
@mlpack-jenkins test this please
hm, currently we test against shogun 6.0.0 (https://github.com/mlpack/benchmarks/blob/master/libraries/package-urls.txt), maybe hierarchical clustering was introduced in > 6.0.0?
It says "no module named hierarchical_clustering" I feel it's some bad naming on my part Let me just check once again and get back with the suggestions incorporated as well
I have no clue as to why the MATLAB tests are failing though Didn't change anything there
Sorry, that was my fault, I rebooted the benchmarking test systems and did not make sure the MATLAB tunnels came back up correctly. I'll try the build again...
@mlpack-jenkins test this please
I had run tests.py locally and it hadn't thrown any error I can't figure out why it can't find the module Could you once check if there are any path/naming issues? I'm on it as well
The MATLAB tests are now working.
@chogba I'm not sure this is a path issue, since every other shogun test runs fine; do you also test with shogun 6.0.0?
I left a few comments, let me know what you think. Happy to add more things to benchmark, so thank you for the contribution. :)
@zoq the src folder of shogun 6.0.0 shows a Hierarchical.cpp+.h combination so I think Hierarchical clustering is a part of the module @karlnapf is that correct?
Okay, I think if you adjust the path as I pointed out above the test should run just fine.
@zoq @rcurtin kindly review now
@mlpack-jenkins test this
my bad again
should be fine now, sorry for the trouble
@mlpack-jenkins add to whitelist
Was there still a problem?
@zoq @rcurtin kindly let me know if there's an issue
@zoq @rcurtin any updates?
@karlnapf @zoq @rcurtin kindly update, so that i can proceed with your inputs, and contribute further... thank you!
Hello @chogba can you also test the other distances: manhattan, cosine, chebyshev
. Also, it's fine for me to merge this afterwards, we can add additional metrics once we figured out what makes sense in this case. How does this sound?
@zoq works fine! will add the tests for other distance checks too.. Also,I am working on adding a similar benchmark script for another algorithm in the meantime.. Could you provide me with a few more pointers as to where else I can devote my time?
@chogba don't worry, I see every update to every PR in an email. I respond when I have time (there are lots of PRs), so please be patient.
If there are no obvious other metrics, I agree with @zoq that we can wait until later to add them, if necessary, but let's see if @karlnapf has any input on the matter.
Right now the continuous integration for this repository is offline (hopefully it will be back up today or tomorrow); when it is back online, I'll have Jenkins test this and then assuming that all works I think there will be no more comments from my end.
lgtm!
@mlpack-jenkins test this please
Same here! Thx guys!
…r checking the benchmarking script, made corresponding additions to config.yaml and tests/tests.py