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

He Initialization and Lecun Normal initialization #1342

Merged
merged 16 commits into from Apr 17, 2018

Conversation

Projects
None yet
4 participants
@Prabhat-IIT
Contributor

Prabhat-IIT commented Mar 30, 2018

It is in continuation with #1280
I've just corrected minor style mistakes. I think these initializations are need of the hour and should be incorporated as soon as possible.

@Prabhat-IIT

This comment has been minimized.

Contributor

Prabhat-IIT commented Mar 30, 2018

@zoq can you run the travis again?

@Prabhat-IIT

This comment has been minimized.

Contributor

Prabhat-IIT commented Mar 31, 2018

@zoq style check is failing due to svd_impl. Same is the case with SAGA pr. I think it'll need a separate commit.

@zoq

This comment has been minimized.

Member

zoq commented Apr 1, 2018

@mlpack-jenkins test this please

@zoq

zoq approved these changes Apr 3, 2018

@@ -0,0 +1,110 @@
/**
* @file lecun_normal_init.hpp
* @author Dakshit Agrawal and Prabhat Sharma

This comment has been minimized.

@zoq

zoq Apr 3, 2018

Member

Can you split the authors (use @author for each name).

{
// He initialization rule says to initialize weights with random
// values taken from a gaussian distribution with mean = 0 and
// standard deviation = sqrt(1/rows), i.e. variance = (1/rows).

This comment has been minimized.

@zoq

zoq Apr 3, 2018

Member

Super picky style comment, can you use a space between operations?

}
// Multipling a random variable X with variance V(X) by some factor c,
// then the variance V(cX) = (c^2)* V(X).

This comment has been minimized.

@zoq

zoq Apr 3, 2018

Member

See comment above.

* For more information, the following papers can be referred to:
*
* @code
* @inproceedings{conf/nips/KlambauerUMH17,

This comment has been minimized.

@zoq

zoq Apr 3, 2018

Member

Do you mind to reformat the citation, here is an example:

* @code
* @article{Kingma2014,
* author = {Diederik P. Kingma and Jimmy Ba},
* title = {Adam: {A} Method for Stochastic Optimization},
* journal = {CoRR},
* year = {2014},
* url = {http://arxiv.org/abs/1412.6980}
* }
* @endcode

@zoq

zoq approved these changes Apr 7, 2018

If you could handle the issue I pointed out, I think this is ready and I'll go ahead and merge this in 3 days to leave time for any other comments.

@@ -0,0 +1,107 @@
/**
* @file he_init.hpp
* @authors Dakshit Agrawal and Prabhat Sharma

This comment has been minimized.

@zoq

zoq Apr 7, 2018

Member

Can you split up the author section (for each author another line that starts with @author), that way doxygen can pick this up.

* For more information, the following paper can be referred to:
*
* @code
* @article{He2015DelvingDI,

This comment has been minimized.

@zoq

zoq Apr 7, 2018

Member

Do you mind to reformat this as well?

@dakshitagrawal97

This comment has been minimized.

Contributor

dakshitagrawal97 commented Apr 7, 2018

Thanks @Prabhat-IIT for completing this PR! Appreciate it. :)

Fixed minor style issue
Doxygen authors changed to author
@rcurtin

Looks good to me once the citations are reformatted. Thanks @Prabhat-IIT and @dakshitagrawal97. 👍

@Prabhat-IIT

This comment has been minimized.

Contributor

Prabhat-IIT commented Apr 10, 2018

@rcurtin the citations have been already formatted :)

@rcurtin

This comment has been minimized.

Member

rcurtin commented Apr 10, 2018

Can you match the indentation of the citations @zoq provided as an example please?

@Prabhat-IIT

This comment has been minimized.

Contributor

Prabhat-IIT commented Apr 10, 2018

Sorry, @rcurtin I didn't see the minute spacing details :(

@Prabhat-IIT

This comment has been minimized.

Contributor

Prabhat-IIT commented Apr 12, 2018

@zoq travis failed once due to time out. I think you can merge this after initiating travis build once again

@zoq zoq merged commit 10ce8d9 into mlpack:master Apr 17, 2018

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Memory Checks
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@zoq

This comment has been minimized.

Member

zoq commented Apr 17, 2018

Thanks again for another great contribution.

@Prabhat-IIT Prabhat-IIT deleted the Prabhat-IIT:newinit branch Apr 17, 2018

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