Skip to content

Fixed the lpnormpool implementation #639

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

IBArbitrary
Copy link

@IBArbitrary IBArbitrary commented Jun 6, 2025

The lpnormpool implementation was wrong. The existing implementation is y = (∑ᵢ xᵢ^p)^(1 / p), whereas the true Lp norm shuld be y = (∑ᵢ |xᵢ|^p)^(1 / p), where |x| is abs(x). This also affects the gradient, which should be ∂y/∂xᵢ = |xᵢ|^(p-1) × y^(1-p) × sign(xᵢ) instead of ∂y/∂xᵢ = xᵢ^(p-1) × y^(1-p). The necessary changes should go into src/impl/pooling_direct.jl which is what this PR achieves.

The existing implementation is exact only for the input array x such that xᵢ >= 0 ∀ xᵢ ∈ x. This will not generally be the case. For example, when the input to the pooling layer comes from a layer which had leaky ReLU activation, then the problem of the implementation would manifest.

MWE of the problem:

x = randn(4, 4, 4, 4)
lpnormpool(x, 0.5, (2, 2))
> ERROR: DomainError with -0.21106262688542637:
> Exponentiation yielding a complex result requires a complex argument.
> Replace x^y with (x+0im)^y, Complex(x)^y, or similar.

Implemented the correct LpNorm Pooling and backprop steps
@mcabbott
Copy link
Member

Original from @skyleaworlder, care to review?

PR needs tests. At present only p=1,2 are tested, perhaps that's all that this was intended to support?

@IBArbitrary
Copy link
Author

PR needs tests. At present only p=1,2 are tested, perhaps that's all that this was intended to support?

Forgive me if I'm mistaken, I am not familiar with tests. I found this line in /test/pooling.jl. The p values being tested are half integer. It works there because the arrays being pooled are also positive. But that is not the general case is what I feel. For integer values of p, the pooling would work for non positive arrays as well, but why I assumed its supposed to support any values is because p was typecasted as Real.

I belive my implementation is faithful to the definition of Lp norm. If you can give me resources or tips to construct tests, I'll add that to my fork.

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

Successfully merging this pull request may close these issues.

2 participants