added logistic regression classifier#1227
added logistic regression classifier#1227aceveggie wants to merge 39 commits intoopencv:masterfrom aceveggie:master
Conversation
|
@mdim, can you review this PR, it seems to be fine with buildbot tests. |
|
@vkocheganov That's weird. My PR didn't have warnings before, it had all greens. Do I have to do anything? |
|
Yeah this is indeed weird, will investigate what was the problem. Thanks @aceveggie. @mdim , could you please take a look at it? |
|
Fixed trailing white-spaces in above commit. Its weird that the http://pullrequest.opencv.org/ page showed all successes and all green. But, when I looked in "Doc" link, it showed a warning saying that there have been trailing white-space in my code. Anyways, I fixed it. |
|
I wonder if anyone is reviewing this? @vkocheganov |
|
Sorry for the delay, @aceveggie. @kirill-kornyakov Kirill, please, have a look also. |
|
@mdim, do you think you will have a chance to review it? |
|
@kirill-kornyakov @snosov1 Its been more than 3 weeks. I wonder if anyone actually looked at the code? Is there anything I could help to make it faster (like more documentation on the algorithm developed, etc.)? This algorithm is present in other ML libraries in Python(sklearn), Java (Weka), etc but missing in OpenCV. I've used this algorithm for my research work and classifying MNIST digits in Machine Learning class (on coursera.org). |
|
Sorry, but the issue is the same as here: #1188 (comment) |
|
@aceveggie Sorry for the long response! I hope to find a time to review the PR by the end of the week. Now most of OpenCV team supports the lib in free time, so the progress is not as fast as we want. Also reviewing new functionality really takes much time (vs eg bugfix). |
modules/ml/include/opencv2/ml.hpp
Outdated
There was a problem hiding this comment.
Could you please rename minibatchsize? (eg mini_batch_size)
There was a problem hiding this comment.
Yes, remove it. Some of such macros are used for automatic python and java wrappers generation. But don't forget about CV_EXPORTS. Please read about this here http://code.opencv.org/projects/opencv/wiki/Coding_Style_Guide (useful link) and be sure that you follow this coding style.
There was a problem hiding this comment.
I will do it. Are the names such as "CV_PROP_RW" serve any special purpose. I can't find them in docs.opencv.org. I can remove those too? Are these variables in C style? Please comment.
There was a problem hiding this comment.
You can find "CV_PROP_RW" in both C structures and C++ classes of OpenCV. "CV_PROP_RW" is a special marker used by automatic OpenCV python wrappers generator. "CV_PROP_RW" means that this is a property which you can read and write in python.
There was a problem hiding this comment.
And, yes, remove it. You should not care about python/java generators. But as I said before don't forget about CV_EXPORTS.
There was a problem hiding this comment.
In that case, I will remove it. It doesn't make sense, I have made no effort in making this compatible with python.
|
@mdim: made requested changes to API and appropriate changes in documentation too. Just checked build-bot status. It showed all successes! 👍 |
|
Here is a comparative analysis with Naive Bayes classifier (for Logistic Regression). http://select.cs.cmu.edu/class/10701-F09/slides/logistic-regression-annotated.pdf |
|
@aceveggie , @mdim what a patience you have guys to deal with such a big PR :-) P.s. .pdf presentation? @aceveggie , you are really fan of what you do :-) and it's really great! |
|
@vkocheganov : LOL :). I love OpenCV! I learnt a great deal(internal workings) about OpenCV by doing this PR. Also, this is my first ever PR on Github (or anywhere else). So, I'm very passionate. I'm trying to patient here. @mdim has been very patient with my C++ skills here. I deal with multiple programming languages at a time. So, I tend to forget the best practices in a programming language(hence, the long conversations :)). The pdf presents a comparative analysis of LR classifier with Naive Bayes. I just don't want this PR to be shrugged off as something that is not necessary in OpenCV. This classifier will be a great addition to OpenCV. |
|
I see, all you are saying @aceveggie makes sense! Keep it up, @aceveggie ! |
|
@mdim Sending ping to you:) This contribution could be a marvelous New Year present for OpenCV! |
|
I would love to see some progress on this! |
|
@mdim, should we take it? Or there is something else you want to see improved? |
fixed warnings in type conversions from size_t to int (in getting size of number of unique classes in a training problem).
|
fixed other type conversion warning. Now build status is showing all success. |
|
@mdim all green, your move |
|
@mdim do you want me to test this against existing Logistic Regression implementations anywhere and report recognition accuracy? I guess that would make your job easier. |
|
There is failed OCL builder. Lets re-run builders. |
|
This has been almost an year. |
|
Nice contribution, it comes in really handy! However, http://pullrequest.opencv.org/ now only shows "not_queued" for all the builds - is there something wrong? |
|
ML has been recently refactored; I agree that LR is a useful tool; I think, it should be refactored to match the style of new ml |
|
@aceveggie, once again, thank you very much for the PR! And very, very sorry for delay; it has been another difficult period; finally we managed to merge it in. |
|
@alalek: I can't seem to find the exact messages exchanged previously on why the existing licensing choice was used (Intel open source license). I can change it to the recommended license (if I can do that). Please let me know. |
|
I have prepared PR with updated OpenCV license headers here: #19741 |
Logistic Regression is supervised learning technique. It can be used for digit recognition, action recognition, etc. A class for logistic regression has been added. A sample program, documentation and test for logistic regression has also been added.