FluxML
Fixed the lpnormpool implementation
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.
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?
PR needs tests. At present only
p=1,2are 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.