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

Added LP pooling layer #2828

Merged
merged 12 commits into from
Feb 10, 2021
Merged

Added LP pooling layer #2828

merged 12 commits into from
Feb 10, 2021

Conversation

abhinav-anand-addepar
Copy link
Member

@abhinav-anand-addepar abhinav-anand-addepar commented Feb 5, 2021

Comment on lines 48 to 52
const size_t kernelWidth,
const size_t kernelHeight,
const size_t strideWidth = 1,
const size_t strideHeight = 1,
const bool floor = true);
Copy link
Member

Choose a reason for hiding this comment

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

The parameter should align with the first one.

{
const arma::Mat<eT>& inputArea = input(arma::span(i, i + rStep - 1),
arma::span(j, j + cStep - 1));
size_t sum = arma::pow(arma::accu(arma::pow(inputArea, norm_type)), (norm_type-1) / norm_type);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this exceeds the 80 line character limit.

src/mlpack/methods/ann/layer/lp_pooling.hpp Outdated Show resolved Hide resolved
src/mlpack/tests/ann_layer_test.cpp Outdated Show resolved Hide resolved
src/mlpack/tests/ann_layer_test.cpp Outdated Show resolved Hide resolved
@abhinav-anand-addepar
Copy link
Member Author

@zoq the build is failing and i am not able to figure it out.

@@ -282,6 +283,7 @@ using LayerTypes = boost::variant<
LSTM<arma::mat, arma::mat>*,
MaxPooling<arma::mat, arma::mat>*,
MeanPooling<arma::mat, arma::mat>*,
LpPooling<arma::mat, arma::mat>*,
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the layer to MoreTypes (same file) instead and see if that works, we might run into some limitations here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it worked.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, great, once #2777 is ready this is going to be a lot easier.

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

Looks good to me, for future contributions it would be useful to use more descriptive commit messages, I know it's not fun but incredibly useful for someone to figure out what changed between commits.

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

Copy link
Member

@kartikdutt18 kartikdutt18 left a comment

Choose a reason for hiding this comment

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

Hey @abh2k, Thanks for taking the time to contribute this PR. This looks good to me. I have mentioned minor style fixes.
Regards.

src/mlpack/methods/ann/layer/lp_pooling.hpp Outdated Show resolved Hide resolved
src/mlpack/methods/ann/layer/lp_pooling.hpp Outdated Show resolved Hide resolved
Co-authored-by: kartikdutt18 <39593019+kartikdutt18@users.noreply.github.com>
@zoq zoq merged commit 9743441 into mlpack:master Feb 10, 2021
@zoq
Copy link
Member

zoq commented Feb 10, 2021

Thanks again for the contribution.

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

Successfully merging this pull request may close these issues.

None yet

3 participants