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

Add EvaluateWithGradient To FFN and RNN #1467

Merged
merged 2 commits into from Jul 15, 2018

Conversation

Projects
None yet
3 participants
@ShikharJ
Member

ShikharJ commented Jul 11, 2018

No description provided.

typename... CustomLayers>
template<typename GradType>
double FFN<OutputLayerType, InitializationRuleType, CustomLayers...>::
EvaluateWithGradient(const arma::mat& /* parameters */,

This comment has been minimized.

@zoq

zoq Jul 12, 2018

Member

Do you think the existing Gradient method could use the EvaluateWithGradient as well?

@zoq

zoq Jul 12, 2018

Member

Do you think the existing Gradient method could use the EvaluateWithGradient as well?

This comment has been minimized.

@ShikharJ

ShikharJ Jul 12, 2018

Member

@zoq It could, but there would be practically no speedup, as the same functions would be required to get called.

@ShikharJ

ShikharJ Jul 12, 2018

Member

@zoq It could, but there would be practically no speedup, as the same functions would be required to get called.

This comment has been minimized.

@zoq

zoq Jul 12, 2018

Member

Right, but we could remove some code duplication.

@zoq

zoq Jul 12, 2018

Member

Right, but we could remove some code duplication.

@ShikharJ

This comment has been minimized.

Show comment
Hide comment
@ShikharJ

ShikharJ Jul 12, 2018

Member

@zoq Thanks for the comments! This is ready for review.

Member

ShikharJ commented Jul 12, 2018

@zoq Thanks for the comments! This is ready for review.

@rcurtin

Looks great to me. I like code deduplication. :) Did you happen to see what the speedup was for this? I'd imagine 10-30% for training of general FFNs.

(Also I guess we could do the same thing for RNNs too? I think the code would be really similar.)

Show outdated Hide outdated src/mlpack/methods/ann/ffn_impl.hpp Outdated
Show outdated Hide outdated src/mlpack/methods/ann/ffn_impl.hpp Outdated
@ShikharJ

This comment has been minimized.

Show comment
Hide comment
@ShikharJ

ShikharJ Jul 12, 2018

Member

@rcurtin I'll find what the duration per call of individual functions is. That should give us a good idea of the speedup to be expected.

Member

ShikharJ commented Jul 12, 2018

@rcurtin I'll find what the duration per call of individual functions is. That should give us a good idea of the speedup to be expected.

@ShikharJ ShikharJ changed the title from Add EvaluateWithGradient To FFN to Add EvaluateWithGradient To FFN and RNN Jul 12, 2018

@ShikharJ

This comment has been minimized.

Show comment
Hide comment
@ShikharJ

ShikharJ Jul 13, 2018

Member

@zoq A final review?

Member

ShikharJ commented Jul 13, 2018

@zoq A final review?

@zoq

zoq approved these changes Jul 13, 2018

This looks good to me. I'll go ahead and merge this in 2 days to leave time for any other comments. Thanks!

// Get the gradients of the Discriminator.
discriminator.Gradient(discriminator.parameter, i, gradientDiscriminator,
batchSize);
double res = discriminator.EvaluateWithGradient(discriminator.parameter,

This comment has been minimized.

@zoq

zoq Jul 13, 2018

Member

Nice, that makes the file even shorter.

@zoq

zoq Jul 13, 2018

Member

Nice, that makes the file even shorter.

@zoq zoq merged commit 139e0a4 into mlpack:master Jul 15, 2018

5 checks passed

Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jul 15, 2018

Member

Thanks, really great contribution.

Member

zoq commented Jul 15, 2018

Thanks, really great contribution.

@ShikharJ ShikharJ deleted the ShikharJ:FFN branch Jul 15, 2018

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