clean up language error and fluffy comments #3633

MikeLing · 2017-02-17T02:57:31Z

This pr is about fixing language error and fluffy comments in this pr #3608. For more information, see @karlnapf 's comments in #3608 (review)

karlnapf

Pls more care and cleanups.
This is a good exercise to produce good API docs, so lets iterate a bit.

Thanks for putting the effort into it

karlnapf · 2017-02-17T12:12:42Z

src/shogun/multiclass/BruteKNNSolver.h

- * The BruteKNNSolver class is a solver class which inherit from KNNSolver class. It will use brute way
- * to classify the object which mean compare the object against all training objects for each prediction.
+ * The BruteKNNSolver class is a solver class and it use brute way to classify the object,
+ * which means test points are compared to all training data for each prediction.


can you pls use the one I wrote in the comment?

"Test points are compared to all training data for each prediction."

karlnapf · 2017-02-17T12:14:14Z

src/shogun/multiclass/CoverTreeKNNSolver.h

- * The CoverTreeKNNSolver class is a solver class which inherit from KNNSolver class. 
- * It use the cover tree to speed up the nearest neighbor search and you can find more
- * detail information on https://en.wikipedia.org/wiki/Cover_tree
+ * The CoverTreeKNNSolver class is a solver class. 


No need for the first sentence (also in the other places)

karlnapf · 2017-02-17T12:16:14Z

src/shogun/multiclass/CoverTreeKNNSolver.h

- * It use the cover tree to speed up the nearest neighbor search and you can find more
- * detail information on https://en.wikipedia.org/wiki/Cover_tree
+ * The CoverTreeKNNSolver class is a solver class. 
+ * It use the cover tree to speed up the nearest neighbor searching process.


Mistakes:

It uses (third person verbs have an s!)

It uses cover trees, or it uses the cover tree algorithm

.... to speed up the nearest neighbour search process (not searching process) ... Also it is not a search process, so rather write: to speed up the nearest neighbour computation

It uses cover

karlnapf · 2017-02-17T12:16:26Z

src/shogun/multiclass/CoverTreeKNNSolver.h

- * detail information on https://en.wikipedia.org/wiki/Cover_tree
+ * The CoverTreeKNNSolver class is a solver class. 
+ * It use the cover tree to speed up the nearest neighbor searching process.
+ * For more information, see [https://en.wikipedia.org/wiki/Cover_tree]


why the square brackets around the link?

karlnapf · 2017-02-17T12:16:35Z

src/shogun/multiclass/KDTreeKNNSolver.h

@@ -19,7 +19,7 @@ namespace shogun
 * The KDTREEKNNSolver class is a solver class which inherit from KNNSolver class.
 * It use a k-d tree (short for k-dimensional tree) to delivery the nearest neighbor searches
 * involving the multidimensional search key.
- * Check https://en.wikipedia.org/wiki/K-d_tree for more detail about K-D tree.
+ * For more information, see [https://en.wikipedia.org/wiki/K-d_tree]


karlnapf · 2017-02-17T12:16:43Z

src/shogun/multiclass/KDTreeKNNSolver.h

@@ -19,7 +19,7 @@ namespace shogun
 * The KDTREEKNNSolver class is a solver class which inherit from KNNSolver class.


This stuff needs change as well

karlnapf · 2017-02-17T12:17:02Z

src/shogun/multiclass/KNNSolver.h

- * The KNNSolver class is the virtual base class of BruteKNNSolcer, CoverTreeKNNSolver, KDTreeKNNsolver, LSHKNNSolver. 
- * The KNNSolver class will include most of Variables which been used all the other Solver and methods been used to choose 
- * the index of the most frequent class.
+ * The KNNSolver class is the virtual base class of all the other solvers.


"Virtual base class for all KNN solvers"

No need to put the class name in the description of a class, the name is obvious from the API

karlnapf · 2017-02-17T12:17:30Z

src/shogun/multiclass/LSHKNNSolver.h

@@ -16,9 +16,9 @@ namespace shogun
 {

 /**
- * The LSHKNNSolver class is a solver class which inherit from KNNSolver class.
+ * The LSHKNNSolver class is a solver class.


No need for this,

karlnapf · 2017-02-17T12:17:55Z

src/shogun/multiclass/LSHKNNSolver.h

 * It use LSH(short for Locality-sensitive hashing) to do the nearest neighbor search.
- * More detail information can be found in https://en.wikipedia.org/wiki/Locality-sensitive_hashing.
+ * More detailed information can be found in https://en.wikipedia.org/wiki/Locality-sensitive_hashing.


Can you make this unified with above references

For more information, see ...

MikeLing · 2017-02-17T14:34:57Z

Hi @karlnapf, does it look better to you? :)

karlnapf

Getting better.

Can you research and add the computational complexity of each solver (depending on number of points and dimension) and put them in the description?

karlnapf · 2017-02-17T14:50:24Z

src/shogun/multiclass/BruteKNNSolver.h

@@ -16,8 +16,7 @@ namespace shogun
 {

 /**
- * The BruteKNNSolver class is a solver class which inherit from KNNSolver class. It will use brute way
- * to classify the object which mean compare the object against all training objects for each prediction.
+ * Test points are compared to all training data for each prediction.


Prepend: "Standard KNN solver. "

karlnapf · 2017-02-17T14:50:54Z

src/shogun/multiclass/CoverTreeKNNSolver.h

- * The CoverTreeKNNSolver class is a solver class which inherit from KNNSolver class. 
- * It use the cover tree to speed up the nearest neighbor search and you can find more
- * detail information on https://en.wikipedia.org/wiki/Cover_tree
+ * It uses the cover trees to speed up the nearest neighbour computation.


Cover tree solver. It uses cover trees ...

karlnapf · 2017-02-17T14:51:14Z

src/shogun/multiclass/KDTreeKNNSolver.h

@@ -16,10 +16,9 @@ namespace shogun
 {

 /**
- * The KDTREEKNNSolver class is a solver class which inherit from KNNSolver class.
- * It use a k-d tree (short for k-dimensional tree) to delivery the nearest neighbor searches
+ * It uses k-d tree (short for k-dimensional tree) to speed up the nearest neighbour computation.


KD-tree solver. I uses kd-trees to speed up....

karlnapf · 2017-02-17T14:51:29Z

src/shogun/multiclass/LSHKNNSolver.h

- * The LSHKNNSolver class is a solver class which inherit from KNNSolver class.
- * It use LSH(short for Locality-sensitive hashing) to do the nearest neighbor search.
- * More detail information can be found in https://en.wikipedia.org/wiki/Locality-sensitive_hashing.
+ * It uses LSH(short for Locality-sensitive hashing) to do the nearest neighbour computation.


LSH solver.

space between LSH and (

MikeLing · 2017-02-18T01:54:22Z

Hi @karlnapf , I just fix the existing comments. Maybe we could add the computational complexity later?

karlnapf · 2017-02-18T12:00:49Z

Ok lets do complexity later

karlnapf · 2017-02-18T12:01:16Z

Use git amend to correct commits, now you need to squash them (we want only one)

karlnapf · 2017-02-18T12:02:51Z

src/shogun/multiclass/KDTreeKNNSolver.h

- * involving the multidimensional search key.
- * Check https://en.wikipedia.org/wiki/K-d_tree for more detail about K-D tree.
+ * KD-tree solver. It uses k-d tree (short for k-dimensional tree) to speed up the 
+ * nearest neighbour computation. involving the multidimensional search key.


Sentences start with capital letter. The second sentence misses a subject. I also dont understand what that is supposed to mean

Maybe just remove

karlnapf · 2017-02-18T12:02:56Z

src/shogun/multiclass/CoverTreeKNNSolver.h

- * The CoverTreeKNNSolver class is a solver class which inherit from KNNSolver class. 
- * It use the cover tree to speed up the nearest neighbor search and you can find more
- * detail information on https://en.wikipedia.org/wiki/Cover_tree
+ * Cover tree solver. It uses the cover trees to speed up the nearest neighbour computation.


remove "the"

MikeLing · 2017-02-18T14:17:52Z

@karlnapf I had fix those nits. Thank you for your reviewing :)

karlnapf

Nice thanks!
you see how minimal, yet much clearer (!), the docs now are? No superfluous words, clear description, nothing redundant, etc

clean up language error and fluffy comments

karlnapf requested changes Feb 17, 2017

View reviewed changes

MikeLing force-pushed the clean_up_KNN branch from 5f10c9f to 73f25db Compare February 17, 2017 14:32

karlnapf requested changes Feb 17, 2017

View reviewed changes

karlnapf reviewed Feb 18, 2017

View reviewed changes

clean up language error and fluffy comments

937cb42

MikeLing force-pushed the clean_up_KNN branch from 41c19b4 to 937cb42 Compare February 18, 2017 14:16

karlnapf approved these changes Feb 19, 2017

View reviewed changes

karlnapf merged commit a28c407 into shogun-toolbox:develop Feb 19, 2017

karasikov pushed a commit to karasikov/shogun that referenced this pull request Apr 15, 2017

Merge pull request shogun-toolbox#3633 from MikeLing/clean_up_KNN

3e8d62c

clean up language error and fluffy comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean up language error and fluffy comments #3633

clean up language error and fluffy comments #3633

MikeLing commented Feb 17, 2017

karlnapf left a comment

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

MikeLing commented Feb 17, 2017

karlnapf left a comment

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

karlnapf Feb 17, 2017

MikeLing commented Feb 18, 2017

karlnapf commented Feb 18, 2017

karlnapf commented Feb 18, 2017

karlnapf Feb 18, 2017

karlnapf Feb 18, 2017

karlnapf Feb 18, 2017

MikeLing commented Feb 18, 2017

karlnapf left a comment

		@@ -19,7 +19,7 @@ namespace shogun
		* The KDTREEKNNSolver class is a solver class which inherit from KNNSolver class.

clean up language error and fluffy comments #3633

clean up language error and fluffy comments #3633

Conversation

MikeLing commented Feb 17, 2017

karlnapf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeLing commented Feb 17, 2017

karlnapf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeLing commented Feb 18, 2017

karlnapf commented Feb 18, 2017

karlnapf commented Feb 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeLing commented Feb 18, 2017

karlnapf left a comment

Choose a reason for hiding this comment