ELM update #845
ELM update #845
Conversation
|
Hi there, thanks for the contributions. Before I review this, please add tests and ensure that the code conforms to the mlpack style guidelines. |
|
I will make sure that it conforms to the mlpack style guidelines. Presently
it doesn't.
…On Sun, Dec 25, 2016 at 9:23 PM, Ryan Curtin ***@***.***> wrote:
Hi there, thanks for the contributions. Before I review this, please add
tests and ensure that the code conforms to the mlpack style guidelines.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#845 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJ4bFOn_ZzpwbQIseB4Hb5Vj95GZ_1oyks5rLpF_gaJpZM4LVa9h>
.
_______________________________________________
mlpack mailing list
***@***.***
http://knife.lugatgt.org/cgi-bin/mailman/listinfo/mlpack
|
|
|
||
| namespace mlpack { | ||
| namespace elm { | ||
|
|
There was a problem hiding this comment.
Please add comment at here, explain the purpose of this class and give some example. You can take softmax as example.
|
|
||
| class ELM | ||
| { | ||
| private: |
There was a problem hiding this comment.
Please make your style conform to requirements of mlpack. Like naming rule, how many space should be used etc.
You can check the details at here.
examples :
In mlpack, class member should names as
"trainTime", "weight" "NPrime" and so on.
space before private and public should be one.
space before parameter should be two
| double Test_Accuracy; | ||
|
|
||
| public: | ||
| void Set_Dim(uint16_t Nh, uint16_t D, uint16_t N); |
There was a problem hiding this comment.
member name should be SetDim and so on
There was a problem hiding this comment.
Besides, please write test cases for your class.You can use softmax as example too.
There was a problem hiding this comment.
This is an old initial commit. Please review the new code
| class ELM | ||
| { | ||
| private: | ||
| uint16_t N_prime; //Number of Hidden Neurons |
There was a problem hiding this comment.
Any reason to pick uint16_t but not size_t?
There was a problem hiding this comment.
No it's fine to use size_t !!
| public: | ||
| Elm(const arma::mat& predictors, | ||
| const arma::mat& responses, | ||
| const size_t act=0, |
There was a problem hiding this comment.
Where are the data member of act, Nh, N, D? Do you compile your codes before you commit it?
Please do not omit the error/warning messages of your compiler, they are important and helpful .
If possible, please change the parameter to longer, readable name, like
"hiddenNeurons, dataPoints, dimension".
I like the way you change x_train, y_train to predictors and responses, this make things more readable 👍
|
|
||
| const size_t act = CLI::GetParam<size_t>("Activation_type"); | ||
|
|
||
| elm.Nh = Nh; |
There was a problem hiding this comment.
Please do not make your data member as global.
| */ | ||
|
|
||
| #ifndef MLPACK_METHODS_Elm_HPP | ||
| #define MLPACK_METHODS_Elm_HPP |
There was a problem hiding this comment.
Please keep all of the characters as upper letter
|
|
||
| switch(act) | ||
| { | ||
| case 0 :for(size_t i=0; i<H.n_rows; ++i) |
There was a problem hiding this comment.
Rather than using 0,1,2,3, why not we make it as enum?It is easier to read
enum class Act
{
sigmoid,
sine,
hardlim,
triangularBias,
radialBasis
};
switch(act)
{
case sigmoid : //......
}
| { | ||
| arma::mat weight = arma::randu<arma::mat>(Nh); | ||
| arma::mat bias = arma::randu<arma::mat>(Nh); | ||
| arma::mat beta = arma::randu<arma::mat>(Nh); |
There was a problem hiding this comment.
What are the purpose of these temporary parameters doing(weight, bias, beta)?
If they are no special reasons to put them at here, please remove them.
If you do have some special reasons, please write it down, because this is unusual and
may make other maintainer confuse. Thanks
| default : Log::Fatal << "Please select a suitable activation function to proceed:" << std::endl; | ||
| } | ||
|
|
||
| mat H_inv = pinv(H); // Moore-Penrose pseudo-inverse of matrix H |
There was a problem hiding this comment.
Please make them as const if the value would not be changed. const is a simple yet powerful abstraction mechanism to help you write high quality codes.
|
|
||
| mat predictions = H * beta; // predict the output of ELM | ||
| arma::mat Elm_output = arma::randu<arma::mat>(N); | ||
| Elm_output = predictions; |
There was a problem hiding this comment.
What are the purposes of this temporary parameter?
There was a problem hiding this comment.
Please do not save the output to the csv after predict but let the user have the output results directly.
I suggest you change the api to
void ELM::Predict(const arma::mat& points, arma::mat& predictions);
|
|
||
| void ELM::Train(const arma::mat& predictors, | ||
| const arma::mat& responses, | ||
| const size_t act) |
There was a problem hiding this comment.
If this function do not alter the value of data member, please make this as const function
| } | ||
|
|
||
| mat H_inv = pinv(H); // Moore-Penrose pseudo-inverse of matrix H | ||
| beta = H_inv * responses; //Calculate output weights |
There was a problem hiding this comment.
Where are this beta come from?
| beta = H_inv * responses; //Calculate output weights | ||
| mat trainingOutput = H * beta; // Calculate training accuracy | ||
| vec temp = responses - trainingOutput; | ||
| double trainError = stddev(temp); //calculate training error |
There was a problem hiding this comment.
What are the purposes of this train function?It do not change any states of the class nor provide any output.
|
Closing for inactivity. If you'd like to keep working on this we can reopen it of course. |
Please review it and let me know