Skip to content
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

Enhance class SoftmaxRegression #456

Merged
merged 2 commits into from Sep 28, 2015

Conversation

stereomatchingkiss
Copy link
Contributor

1 : add function Serialize to class SoftmaxRegression
2 : add function Train to class SoftmaxRegression
3 : fix bug--did not initialize fitIntercept

The test case "SoftmaxRegressionFunctionEvaluate" always fail on my machine

2 : add function Train
3 : fix bug--did not initialize fitIntercept
inputSize{inputSize},
numClasses{numClasses},
lambda{0.0001},
fitIntercept{fitIntercept}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, do you mind if I change this to () initialization vs. {} initialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem(I use {} to avoid narrow conversion)

@rcurtin
Copy link
Member

rcurtin commented Sep 28, 2015

Looks good to me; I made a few comments. Let me know what you think. I'll add a test for SoftmaxRegression::Serialize() once I merge. Do you mind if I add your name/email to the list of authors? If that's okay, which name/email should I use?

@stereomatchingkiss
Copy link
Contributor Author

Do you mind if I add your name/email to the list of authors?
My pleasure

name : Tham Ngap Wei
email : thamngapwei@gmail.com

Thanks for adding the unit test.

rcurtin added a commit that referenced this pull request Sep 28, 2015
@rcurtin rcurtin merged commit 96af50b into mlpack:master Sep 28, 2015
@rcurtin
Copy link
Member

rcurtin commented Sep 28, 2015

Okay, I made some simple changes in cbeb3ea..f85a9b2; let me know if you see anything wrong. If you are interested, here are some future possibilities for refactoring/improving the SoftmaxRegression code further:

  • Storing inputSize and numClasses is, I think, redundant, and I think you can just get these from the parameters matrix. So the constructors can be cleaned up a little (although numClasses will still be necessary in the constructor).
  • The type of label would be better as arma::Row<size_t> not arma::vec.
  • The InitializeWeights() function could be made static or moved elsewhere so that creating a SoftmaxRegressionFunction to initialize the weights in the non-training constructor isn't necessary.
  • A softmax_regression_main.cpp program could be written that could allow users to use softmax regression from the command line.

Don't feel obligated to do any of these -- I'm just tossing them out there as ideas. :)

Thanks for your contribution! :)

@stereomatchingkiss
Copy link
Contributor Author

Your suggestions are good, about

Storing inputSize and numClasses is, I think, redundant

Some(or many) classes of mlpack always ask for the inputSize(feature size) from the users, I think they could be removed too, because many algorithms should be able to find out the inputSize from the training data

Thanks for your contribution! :)
You are welcome:), thanks for the helps from community too

@stereomatchingkiss stereomatchingkiss deleted the softmax_enhance branch September 29, 2015 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants