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

Logistic Regression #293

Closed
rcurtin opened this Issue Dec 29, 2014 · 8 comments

Comments

Projects
None yet
1 participant
@rcurtin
Member

rcurtin commented Dec 29, 2014

Reported by sumedhghaisas on 12 Oct 43750599 09:03 UTC
Implementation Of Logistic Regression with l2-regularization

Files -
logistic_regression.hpp
logistic_regression_impl.hpp
logistic_regression_function.hpp
logistic_regression_function_impl.hpp

Implemented a regression class which takes optimizer as a template and implemented methods to support both L_BFGS and SGD optimization.

@rcurtin rcurtin self-assigned this Dec 29, 2014

@rcurtin rcurtin added this to the mlpack 1.0.8 milestone Dec 29, 2014

@rcurtin rcurtin closed this Dec 29, 2014

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by rcurtin on 1 Nov 43758210 08:02 UTC
Hello Sumedh,

Thanks for the contribution!

I'm actually going to move this to 1.0.8 because I don't have time to look into it before I release 1.0.7 (which should be today or tomorrow). I should find time in the next two weeks or so to go over the code, though.

Member

rcurtin commented Dec 30, 2014

Commented by rcurtin on 1 Nov 43758210 08:02 UTC
Hello Sumedh,

Thanks for the contribution!

I'm actually going to move this to 1.0.8 because I don't have time to look into it before I release 1.0.7 (which should be today or tomorrow). I should find time in the next two weeks or so to go over the code, though.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by rcurtin on 9 Jul 43857182 19:24 UTC
Hello Sumedh,

This has taken a lot longer than I thought; I have been a lot busier than I had hoped. :(

I've committed your code in r16011, and I'm in the process of going through it and testing it. If you've written a test, I'll happily take it, but I don't think it will be too much trouble for me to write some. You can keep track of the changes I'm making by watching the src/mlpack/methods/logistic_regression/ directory in svn trunk. I'll summarize what I've done when I'm finished in this ticket, when I resolve it.

Thanks,

Ryan

Member

rcurtin commented Dec 30, 2014

Commented by rcurtin on 9 Jul 43857182 19:24 UTC
Hello Sumedh,

This has taken a lot longer than I thought; I have been a lot busier than I had hoped. :(

I've committed your code in r16011, and I'm in the process of going through it and testing it. If you've written a test, I'll happily take it, but I don't think it will be too much trouble for me to write some. You can keep track of the changes I'm making by watching the src/mlpack/methods/logistic_regression/ directory in svn trunk. I'll summarize what I've done when I'm finished in this ticket, when I resolve it.

Thanks,

Ryan

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by sumedhghaisas on 15 Dec 43894935 18:56 UTC
Sorry for such a late reply I got stuck up in many of college work... I am currently going through many of the changes you have made ....

Member

rcurtin commented Dec 30, 2014

Commented by sumedhghaisas on 15 Dec 43894935 18:56 UTC
Sorry for such a late reply I got stuck up in many of college work... I am currently going through many of the changes you have made ....

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by sumedhghaisas on 8 Sep 43895036 17:02 UTC
In changeset 16020 you have changed the vector implementation of the logistic function into a for loop.. Wouldn't the vector implementation will give less computational overhead?? Your point of neglecting the dividing factor of number of features is correct... if we do remove it the vector implementation of that equation would be

-(sum(arma::trans(responses) * arma::log(sigmoid)) +
sum(arma::trans(arma::onesarma::vec(nCols, 1) - responses) *
arma::log(arma::onesarma::vec(nCols,1) - sigmoid)))
+ regularization;

and u inverted both the result and the regularization for minimization purpose... but after deriving the regularized LR we get the sign of the regularization term as positive.. so it should be added to the term (- result).

Member

rcurtin commented Dec 30, 2014

Commented by sumedhghaisas on 8 Sep 43895036 17:02 UTC
In changeset 16020 you have changed the vector implementation of the logistic function into a for loop.. Wouldn't the vector implementation will give less computational overhead?? Your point of neglecting the dividing factor of number of features is correct... if we do remove it the vector implementation of that equation would be

-(sum(arma::trans(responses) * arma::log(sigmoid)) +
sum(arma::trans(arma::onesarma::vec(nCols, 1) - responses) *
arma::log(arma::onesarma::vec(nCols,1) - sigmoid)))
+ regularization;

and u inverted both the result and the regularization for minimization purpose... but after deriving the regularized LR we get the sign of the regularization term as positive.. so it should be added to the term (- result).

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by rcurtin on 14 Apr 43902262 02:48 UTC
Hey,

Thanks for double-checking my work. :)

In MATLAB writing things as vectorized can often result in much faster evaluation, but that doesn't apply so much in this case. Internal Armadillo code often uses for loops in the exact same way I did. What I'm trying to avoid is the allocation and filling of a big vector of ones, when that isn't actually necessary to get the correct result. When I have a main executable running (I'm working on this now, albeit a little slowly), I'll test both implementations and report my results back, but I'm nearly certain that in this case the non-vectorized implementation will be faster.

You are correct about the regularization, though. Thank you for pointing it out, because the tests were also written incorrectly. I've fixed the issues in r16065 and r16066.

Member

rcurtin commented Dec 30, 2014

Commented by rcurtin on 14 Apr 43902262 02:48 UTC
Hey,

Thanks for double-checking my work. :)

In MATLAB writing things as vectorized can often result in much faster evaluation, but that doesn't apply so much in this case. Internal Armadillo code often uses for loops in the exact same way I did. What I'm trying to avoid is the allocation and filling of a big vector of ones, when that isn't actually necessary to get the correct result. When I have a main executable running (I'm working on this now, albeit a little slowly), I'll test both implementations and report my results back, but I'm nearly certain that in this case the non-vectorized implementation will be faster.

You are correct about the regularization, though. Thank you for pointing it out, because the tests were also written incorrectly. I've fixed the issues in r16065 and r16066.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by rcurtin on 22 Sep 43995086 05:00 UTC
Ok, finally got around to finishing the logistic_regression executable in r16087. This'll be included with mlpack 1.0.8. Thanks again for the contribution!

Member

rcurtin commented Dec 30, 2014

Commented by rcurtin on 22 Sep 43995086 05:00 UTC
Ok, finally got around to finishing the logistic_regression executable in r16087. This'll be included with mlpack 1.0.8. Thanks again for the contribution!

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by sumedhghaisas on 29 Feb 43995720 07:16 UTC
So is the non-vectorized implementation is faster in this case?? Is this specific to this case or is generally true for the case of Armadillo library???

Member

rcurtin commented Dec 30, 2014

Commented by sumedhghaisas on 29 Feb 43995720 07:16 UTC
So is the non-vectorized implementation is faster in this case?? Is this specific to this case or is generally true for the case of Armadillo library???

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by rcurtin on 11 Jul 44010409 06:06 UTC
In general with Armadillo, the non-vectorized implementation ends up being the same (or comparable to) the vectorized implementation. In many cases the internal Armadillo implementation (or the internal LAPACK implementation) is a for loop, so writing the same thing in mlpack code should perform about as well. Does that help clarify things?

Member

rcurtin commented Dec 30, 2014

Commented by rcurtin on 11 Jul 44010409 06:06 UTC
In general with Armadillo, the non-vectorized implementation ends up being the same (or comparable to) the vectorized implementation. In many cases the internal Armadillo implementation (or the internal LAPACK implementation) is a for loop, so writing the same thing in mlpack code should perform about as well. Does that help clarify things?

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