Add `lppool` implementation #447

skyleaworlder · 2022-12-24T01:42:11Z

About FluxML/Flux.jl#1431: LPPool1d and LPPool2d.

PR Checklist

Related tests and documentation will come soon.

Tests are added
Documentation, if applicable

mcabbott

Looks good, thanks! I have not checked the implementation closely but have a few comments in the meantime...

mcabbott · 2022-12-27T00:37:19Z

src/pooling.jl

+    lppool(x, p::Number, k::NTuple; pad=0, stride=k)
+
+Perform Lp pool operation with `p`-norm and `window size `k` on input tensor `x`
+"""


Can this explain a bit more?

It should say that it requires ndims(x) == length(k)+2.

I think it should also say lppool(x, 1, k) ./ prod(k) ≈ meanpool(x, k), and maybe lppool(x, 2, k).^2 ./ prod(k) ≈ meanpool(x.^2, k).

What range of p is allowed? E.g. someone might expect lppool(reshape(1:10.0,:,1,1), Inf, (3,)) to be maxpool, but it isn't... maybe anything which works for norm but not here should be an error?

Can it briefly say what types are allowed for stride, pad, or refer to fuller docs elsewhere?

The PyTorch layer docstring would be a good inspiration for this.

Can this explain a bit more?

It should say that it requires ndims(x) == length(k)+2.

I think it should also say lppool(x, 1, k) ./ prod(k) ≈ meanpool(x, k), and maybe lppool(x, 2, k).^2 ./ prod(k) ≈ meanpool(x.^2, k).

What range of p is allowed? E.g. someone might expect lppool(reshape(1:10.0,:,1,1), Inf, (3,)) to be maxpool, but it isn't... maybe anything which works for norm but not here should be an error?

Can it briefly say what types are allowed for stride, pad, or refer to fuller docs elsewhere?

I add docs for these aspects in new commit.

The PyTorch layer docstring would be a good inspiration for this.

I wonder if it might be suitable to write super detailed docs here. Flux.jl would wrap pooling layer after all.

Our docstrings for meanpool and maxpool are quite short. Given that LP-pooling is far less well known however, it warrants a bit more explanation.

src/pooling.jl

…lppool-maxpool

src/impl/pooling_direct.jl

src/pooling.jl

test/pooling.jl

…nd doc

src/pooling.jl

mcabbott · 2022-12-29T17:20:33Z

Re the name, we have meanpool and maxpool, corresponding to mapping mean and maximum (which is reduce(max, ...) over moving windows.

This one maps norm over a moving window. Shouldn't it be normpool?

I realise this breaks with pytorch, but we aren't tied to copying them. Also, lppool is a weird name to read, how many ppp in a row, etc. Obviously we can say "also known as..." so that searching for variations of this name finds you the right function.

ToucheSir · 2022-12-29T18:21:54Z

The PyTorch implementation uses "power-average pooling", so that's another option.

One concerning thing I did find while looking around for name ideas is that nobody seems to be using this? GitHub search turns up only a couple instances of LPPool2d that weren't just copies of the PyTorch repo, and I couldn't identify the source paper/presentation/document this operation was first introduced in.

skyleaworlder · 2023-01-03T14:22:59Z

The PyTorch implementation uses "power-average pooling", so that's another option.

One concerning thing I did find while looking around for name ideas is that nobody seems to be using this? GitHub search turns up only a couple instances of LPPool2d that weren't just copies of the PyTorch repo, and I couldn't identify the source paper/presentation/document this operation was first introduced in.

The paper proposes LPPool might be Learned-Norm Pooling for Deep Feedforward and Recurrent Neural Networks. In this paper, they claim it's better to consider $L_p$ as a hyperparameter instead of a fixed value. And about another problem, I must say I also haven't used this kind of pooling method, maxpool for me mostly. Actually, I just notice Flux.jl#1431 and feel like taking this to learn about Flux.

ToucheSir · 2023-01-03T23:59:21Z

Nice find! Do you mind adding it as the primary reference in the docstring (see how this is done for Flux layers)? Also, is lpnormpool too much of a mouthful, and if so which parts of that name should stay since the paper uses both $L_p$ and "norm"?

skyleaworlder · 2023-01-04T09:57:34Z

Nice find! Do you mind adding it as the primary reference in the docstring (see how this is done for Flux layers)? Also, is lpnormpool too much of a mouthful, and if so which parts of that name should stay since the paper uses both $L_p$ and "norm"?

OK, I've changed the name in recent commit and add paper reference.

mcabbott · 2023-01-04T14:32:58Z

The title does have "Norm Pooling" as they felt squeezing L_p in there too much?

Also LinearAlgebra's function could be called lpnorm, but it's not, they settled on just norm. That seems like the obvious thing to echo.

skyleaworlder · 2023-01-04T15:30:22Z

I think lppool is ok or "lp" is needed because pytorch uses this. If normpool is used as name, users may spend time figuring out that normpool equals lppool in pytorch.

CarloLucibello · 2023-01-05T17:39:50Z

having the more articulate name lpnormpool for a rarely used operator seems fine

src/pooling.jl

skyleaworlder · 2023-01-06T15:27:41Z

@mcabbott Thanks for your patient review! I resolved all points metioned above and re-requested a new review just now :)

skyleaworlder · 2023-01-08T13:40:45Z

@mcabbott @ToucheSir @CarloLucibello Thanks for your patience and kindness again! I really learn alot from this PR.

mcabbott · 2023-01-25T16:47:16Z

src/pooling.jl

@@ -103,7 +109,7 @@ end
 # Finally, let's generate auto-allocating versions of all our functions, for all backends:
 for backend in (Symbol(), :_direct, :_nnpack)
    # First make auto-allocating versions of the basic pooling calls:
-    for name in (:maxpool, :meanpool)
+    for name in (:maxpool, :meanpool, :lpnormpool)
        @eval begin
            function $(Symbol("$(name)$(backend)"))(


Maybe worth noting that this similar means that lower-precision input gives the correct output without promotion, and that integer input fails here (unlike other pooling):

julia> NNlib.lpnormpool(ones(Float32, 4,1,1), 2.001, (2,), stride=1) 3×1×1 Array{Float32, 3}: [:, :, 1] = 1.4139687 1.4139687 1.4139687 julia> NNlib.lpnormpool(ones(Int, 4,1,1), 2.001, (2,), stride=1) ERROR: InexactError: Int64(1.4139686415190424) julia> NNlib.maxpool(ones(Int, 4,1,1), (2,), stride=1) # is OK with integers 3×1×1 Array{Int64, 3}: [:, :, 1] = 1 1 1 julia> NNlib.meanpool(ones(Int, 4,1,1), (2,), stride=1) # is OK with integers 3×1×1 Array{Int64, 3}: [:, :, 1] = 1 1 1

skyleaworlder added 4 commits December 23, 2022 07:56

add: init my lppool impl

9cad84b

add: impl gradient calculate

ccd53cc

test: for lppool(1d & 2d)

07c2d02

doc: juliadoc & formula comment

867f37c

skyleaworlder marked this pull request as ready for review December 26, 2022 18:07

mcabbott reviewed Dec 27, 2022

View reviewed changes

doc: update for meanpool/maxpool, add params details and warning for …

bf94c17

…lppool-maxpool

CarloLucibello reviewed Dec 28, 2022

View reviewed changes

src/impl/pooling_direct.jl Outdated Show resolved Hide resolved

src/impl/pooling_direct.jl Outdated Show resolved Hide resolved

src/impl/pooling_direct.jl Show resolved Hide resolved

src/pooling.jl Outdated Show resolved Hide resolved

test/pooling.jl Outdated Show resolved Hide resolved

update: 1 => T(1); remove unnecessary if-condition; clear error msg a…

671371d

…nd doc

mcabbott reviewed Dec 29, 2022

View reviewed changes

src/pooling.jl Outdated Show resolved Hide resolved

skyleaworlder added 3 commits December 30, 2022 08:14

update: move dim checker from exported pool function to PoolDims

909378c

rename: lppool => normpool

e78bd1b

add: normpool p value checker, p must be in (0, Inf)

7572ea0

skyleaworlder added 2 commits January 4, 2023 08:18

rename: normpool => lpnormpool

d075827

doc: add paper reference to lp pooling

92f0327

CarloLucibello approved these changes Jan 5, 2023

View reviewed changes

mcabbott requested changes Jan 5, 2023

View reviewed changes

src/pooling.jl Outdated Show resolved Hide resolved

src/pooling.jl Outdated Show resolved Hide resolved

src/pooling.jl Outdated Show resolved Hide resolved

mcabbott reviewed Jan 5, 2023

View reviewed changes

src/pooling.jl Outdated Show resolved Hide resolved

src/pooling.jl Outdated Show resolved Hide resolved

mcabbott reviewed Jan 5, 2023

View reviewed changes

src/pooling.jl Outdated Show resolved Hide resolved

src/pooling.jl Outdated Show resolved Hide resolved

src/pooling.jl Outdated Show resolved Hide resolved

src/pooling.jl Outdated Show resolved Hide resolved

doc: usage and parameter description of pool functions

1f6e68f

skyleaworlder requested a review from mcabbott January 6, 2023 15:20

mcabbott merged commit 16b7486 into FluxML:master Jan 7, 2023

mcabbott reviewed Jan 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `lppool` implementation #447

Add `lppool` implementation #447

skyleaworlder commented Dec 24, 2022 •

edited

Loading

mcabbott left a comment

mcabbott Dec 27, 2022

ToucheSir Dec 27, 2022 •

edited

Loading

skyleaworlder Dec 28, 2022 •

edited

Loading

ToucheSir Dec 29, 2022

mcabbott commented Dec 29, 2022 •

edited

Loading

ToucheSir commented Dec 29, 2022

skyleaworlder commented Jan 3, 2023

ToucheSir commented Jan 3, 2023

skyleaworlder commented Jan 4, 2023

mcabbott commented Jan 4, 2023

skyleaworlder commented Jan 4, 2023

CarloLucibello commented Jan 5, 2023

skyleaworlder commented Jan 6, 2023

skyleaworlder commented Jan 8, 2023

mcabbott Jan 25, 2023

Add lppool implementation #447

Add lppool implementation #447

Conversation

skyleaworlder commented Dec 24, 2022 • edited Loading

PR Checklist

mcabbott left a comment

Choose a reason for hiding this comment

mcabbott Dec 27, 2022

Choose a reason for hiding this comment

ToucheSir Dec 27, 2022 • edited Loading

Choose a reason for hiding this comment

skyleaworlder Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

ToucheSir Dec 29, 2022

Choose a reason for hiding this comment

mcabbott commented Dec 29, 2022 • edited Loading

ToucheSir commented Dec 29, 2022

skyleaworlder commented Jan 3, 2023

ToucheSir commented Jan 3, 2023

skyleaworlder commented Jan 4, 2023

mcabbott commented Jan 4, 2023

skyleaworlder commented Jan 4, 2023

CarloLucibello commented Jan 5, 2023

skyleaworlder commented Jan 6, 2023

skyleaworlder commented Jan 8, 2023

mcabbott Jan 25, 2023

Choose a reason for hiding this comment

Add `lppool` implementation #447

Add `lppool` implementation #447

skyleaworlder commented Dec 24, 2022 •

edited

Loading

ToucheSir Dec 27, 2022 •

edited

Loading

skyleaworlder Dec 28, 2022 •

edited

Loading

mcabbott commented Dec 29, 2022 •

edited

Loading