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

extend KNN notebook #3513

Open
karlnapf opened this Issue Nov 5, 2016 · 17 comments

Comments

Projects
None yet
3 participants
@karlnapf
Member

karlnapf commented Nov 5, 2016

Put in speed comparisons with all the different solvers that Shogun has.
Nice and friendly easy entrance task

@sudk1896

This comment has been minimized.

Show comment
Hide comment
@sudk1896

sudk1896 Nov 7, 2016

@karlnapf : Seems like the perfect task for me. I will do this. I saw the kNN notebook in the docs. In it, it only does a speed comparison between KNN_BRUTE and KNN_COVER_TREE. I am assuming you also want me to add a speed comparison between KNN_KDTREE and the rest (There are only 3 solvers that I see in KNN) ? Is that what is required here ?
Also, when you meant the kNN notebook that would be the IPython notebook here right ?
Thanks.

sudk1896 commented Nov 7, 2016

@karlnapf : Seems like the perfect task for me. I will do this. I saw the kNN notebook in the docs. In it, it only does a speed comparison between KNN_BRUTE and KNN_COVER_TREE. I am assuming you also want me to add a speed comparison between KNN_KDTREE and the rest (There are only 3 solvers that I see in KNN) ? Is that what is required here ?
Also, when you meant the kNN notebook that would be the IPython notebook here right ?
Thanks.

@karlnapf

This comment has been minimized.

Show comment
Hide comment
@karlnapf

karlnapf Nov 7, 2016

Member

All yes :)
Generally dop more comparisons, and make the notebook nicer overall. You can browse the web a bit for more inspiration of where KNN is useful and where a fast KNN is useful

Member

karlnapf commented Nov 7, 2016

All yes :)
Generally dop more comparisons, and make the notebook nicer overall. You can browse the web a bit for more inspiration of where KNN is useful and where a fast KNN is useful

@sudk1896

This comment has been minimized.

Show comment
Hide comment
@sudk1896

sudk1896 Nov 8, 2016

@karlnapf : I did investigate usage of KNN_KDTREE, after some Googling I found that it is faster than the others only on low dimensional data. In fact when I ran the tests on the usps dataset, The results were -
Standard KNN took 1.9s
Covertree KNN took 1.0s
KDTree KNN took 3.7s
How do I make changes to the notebook (the multiclass KNN one in doc folder) ? It has a very weird format and I do not know how to add the code to it.
Do you also want me add a plot for the KNN_KDTREE as well like the rest ?
Thanks.

sudk1896 commented Nov 8, 2016

@karlnapf : I did investigate usage of KNN_KDTREE, after some Googling I found that it is faster than the others only on low dimensional data. In fact when I ran the tests on the usps dataset, The results were -
Standard KNN took 1.9s
Covertree KNN took 1.0s
KDTree KNN took 3.7s
How do I make changes to the notebook (the multiclass KNN one in doc folder) ? It has a very weird format and I do not know how to add the code to it.
Do you also want me add a plot for the KNN_KDTREE as well like the rest ?
Thanks.

@karlnapf

This comment has been minimized.

Show comment
Hide comment
@karlnapf

karlnapf Nov 8, 2016

Member

You directly can open the notebook in your browser. Google for ipython notebook and how to use it. Then you send a standard patch on via github pull request. Please clear all output of the notebook before sending it. And also please provide an html link to a fully rendered version (say on gist)

Member

karlnapf commented Nov 8, 2016

You directly can open the notebook in your browser. Google for ipython notebook and how to use it. Then you send a standard patch on via github pull request. Please clear all output of the notebook before sending it. And also please provide an html link to a fully rendered version (say on gist)

@sudk1896

This comment has been minimized.

Show comment
Hide comment
@sudk1896

sudk1896 Nov 9, 2016

@karlnapf : Here is the gist. It will open as an ipython notebook. I have added some text and the code for time comparison for KDTREE. Let me know what else you want.
P.S. The changes I made are in the Accelerating KNN subsection. Also, you would want the PR to contain the ipython (.ipynb) file right ?

sudk1896 commented Nov 9, 2016

@karlnapf : Here is the gist. It will open as an ipython notebook. I have added some text and the code for time comparison for KDTREE. Let me know what else you want.
P.S. The changes I made are in the Accelerating KNN subsection. Also, you would want the PR to contain the ipython (.ipynb) file right ?

@karlnapf

This comment has been minimized.

Show comment
Hide comment
@karlnapf

karlnapf Nov 9, 2016

Member

Can we moive the discussion about your changes into a PR?

Member

karlnapf commented Nov 9, 2016

Can we moive the discussion about your changes into a PR?

@sudk1896

This comment has been minimized.

Show comment
Hide comment
@sudk1896

sudk1896 Nov 9, 2016

Sure, I will submit the ipynb file and we can edit them as we go. Assuming you want me to commit the ipynb file. Right ?

sudk1896 commented Nov 9, 2016

Sure, I will submit the ipynb file and we can edit them as we go. Assuming you want me to commit the ipynb file. Right ?

@karlnapf

This comment has been minimized.

Show comment
Hide comment
@karlnapf

karlnapf Nov 9, 2016

Member

Yes

Member

karlnapf commented Nov 9, 2016

Yes

@sudk1896

This comment has been minimized.

Show comment
Hide comment
@sudk1896

sudk1896 Nov 9, 2016

@karlnapf : It says conflicting file. I replaced the KNN.ipynb file in /doc/ipython-notebooks/multiclass. I assumed that was the right thing to do.

sudk1896 commented Nov 9, 2016

@karlnapf : It says conflicting file. I replaced the KNN.ipynb file in /doc/ipython-notebooks/multiclass. I assumed that was the right thing to do.

@karlnapf

This comment has been minimized.

Show comment
Hide comment
@karlnapf

karlnapf Nov 9, 2016

Member

Google on how to submit pull requests on github

Member

karlnapf commented Nov 9, 2016

Google on how to submit pull requests on github

@sudk1896

This comment has been minimized.

Show comment
Hide comment
@sudk1896

sudk1896 Nov 9, 2016

@karlnapf : I submitted a new PR (the old one was getting too cumbersome).

sudk1896 commented Nov 9, 2016

@karlnapf : I submitted a new PR (the old one was getting too cumbersome).

@karlnapf

This comment has been minimized.

Show comment
Hide comment
@karlnapf

karlnapf Nov 11, 2016

Member

BTW you can always update old PRs, no need to open new ones allt he time

Member

karlnapf commented Nov 11, 2016

BTW you can always update old PRs, no need to open new ones allt he time

@MikeLing

This comment has been minimized.

Show comment
Hide comment
@MikeLing

MikeLing Feb 6, 2017

Contributor

@karlnapf @sudk1896 , I would like to keep working on this issue, due to I just working on the KNN refactoring job. Is that ok?

Contributor

MikeLing commented Feb 6, 2017

@karlnapf @sudk1896 , I would like to keep working on this issue, due to I just working on the KNN refactoring job. Is that ok?

@sudk1896

This comment has been minimized.

Show comment
Hide comment
@sudk1896

sudk1896 Feb 6, 2017

@MikeLing: Go ahead. I'm not working on it anymore.

sudk1896 commented Feb 6, 2017

@MikeLing: Go ahead. I'm not working on it anymore.

@MikeLing

This comment has been minimized.

Show comment
Hide comment
@MikeLing

MikeLing Feb 7, 2017

Contributor

Hi @karlnapf , I create a gist[1] for this issue, please tell me if I need to address it. BTW, I found the KD-Tree is more slower than cover tree and plaint knn, and plain knn. Is that ok to put it into the "Accelerating KNN" session?

[1] https://gist.github.com/MikeLing/6c08b1d5eaebc385c2a63fa1314b13b1

Contributor

MikeLing commented Feb 7, 2017

Hi @karlnapf , I create a gist[1] for this issue, please tell me if I need to address it. BTW, I found the KD-Tree is more slower than cover tree and plaint knn, and plain knn. Is that ok to put it into the "Accelerating KNN" session?

[1] https://gist.github.com/MikeLing/6c08b1d5eaebc385c2a63fa1314b13b1

@karlnapf

This comment has been minimized.

Show comment
Hide comment
@karlnapf

karlnapf Feb 9, 2017

Member

Hi @MikeLing
It is hard to see from a gist what you changed/added. Can you send a PR, it will show me the diff. For that, pls remove the output of the notebook. Then in addition post a gist like the one above, so I can see how it looks.
Then we can discuss in the PR

Member

karlnapf commented Feb 9, 2017

Hi @MikeLing
It is hard to see from a gist what you changed/added. Can you send a PR, it will show me the diff. For that, pls remove the output of the notebook. Then in addition post a gist like the one above, so I can see how it looks.
Then we can discuss in the PR

@MikeLing

This comment has been minimized.

Show comment
Hide comment
@MikeLing

MikeLing Feb 12, 2017

Contributor

@karlnapf here is the PR #3620, please give me some feedback, thank you! :)

Contributor

MikeLing commented Feb 12, 2017

@karlnapf here is the PR #3620, please give me some feedback, thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment