Skip to content

Deprecate conv(u, v::AbstractVector, A) to conv(u, v::Transpose, A) #577

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 11, 2024
Merged

Deprecate conv(u, v::AbstractVector, A) to conv(u, v::Transpose, A) #577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a28138e
Deprecate `conv(u, v::AbstractVector, A)` to `conv(u, v::Transpose, A)`
martinholters Nov 5, 2024
249155f
Test deprecated `conv` method
martinholters Nov 7, 2024
bf14a3c
Rework offset-axes check in three-arg `conv`
martinholters Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/DSP.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module DSP

using FFTW
using LinearAlgebra: mul!, rmul!
using LinearAlgebra: Transpose, mul!, rmul!
using IterTools: subsets

export conv, conv!, deconv, filt, filt!, xcorr
Expand Down
4 changes: 3 additions & 1 deletion src/deprecated.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
import .Util.nextfastfft
@deprecate nextfastfft(ns...) nextfastfft.(ns) false

# deprecations after 0.6
@deprecate (conv(u::AbstractVector{T}, v::AbstractVector{T}, A::AbstractMatrix{T}) where T) conv(u, transpose(v), A)

# deprecations in 0.7
@deprecate freqz(filter::FilterCoefficients{:z}) freqresp(filter, range(0, stop=π, length=250))
@deprecate freqz(filter::FilterCoefficients{:z}, w) freqresp(filter, w)
@deprecate freqs(filter::FilterCoefficients{:s}, w) freqresp(filter, w)
Expand Down
12 changes: 7 additions & 5 deletions src/dspbase.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,19 +787,21 @@
conv(u,v,A)
2-D convolution of the matrix `A` with the 2-D separable kernel generated by
the vectors `u` and `v`.
the vector `u` and row-vector `v`.
Uses 2-D FFT algorithm.
"""
function conv(u::AbstractVector{T}, v::AbstractVector{T}, A::AbstractMatrix{T}) where T
function conv(u::AbstractVector{T}, v::Transpose{T,<:AbstractVector}, A::AbstractMatrix{T}) where T
Copy link
Member Author

Choose a reason for hiding this comment

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

This could also be Adjoint instead of Transpose (with no change to the implementation below). Would that make more sense? Would one rather separate a separable kernel H as H=u*v' or H=u*transpose(v)? (Anyone around working with complex-valued 2D-data and separable filters?)
Or should that even be Union{Adjoint{T,<:AbstractVector}, Transpose{T,<:AbstractVector}} (aka LinearAlgebra.AdjOrTransAbsVec{T})?

Copy link
Member Author

Choose a reason for hiding this comment

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

So I thought I'd ping the author of that method in case he has any insight on whether Transpose or Adjoint would be more idiomatic. Some git blames later, I've arrived at JuliaLang/julia@823cff9 where @JeffBezanson added this to signal.j almost 13 years ago. So Jeff, what do you say?

More seriously, the current code does transpose, so the deprecation for conv(u, v, A) could either be conv(u, transpose(v), A) or conv(u, conj(v)', A), where I find the former more obvious. If we're going with that, it's either Transpose or AdjOrTransAbsVec, and I tend to prefer the simpler Transpose for now. We can always wider the signature later if the need arises.

Another issue that in light of #403, the argument order conv(A, u, transpose(v)) might be more future-proof, as there, the first argument plays the role of the input and the second one plays the role of a convolution kernel (as inspired by MATLAB).

Copy link
Member Author

Choose a reason for hiding this comment

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

Letting this sink in a bit more, I think these concerns are non-issues:

  • As the old code transposed, that's what the deprecation should do. If we want to similarly allow Adjoint, we can always do so later.
  • If another argument order turns out to be more natural later, we can add support for it then. But I see no reason why we should not support the order now implemented -- convolution is commutative and associative, after all -- so keeping that order for the deprecation seems least surprising.

# Arbitrary indexing offsets not implemented
@assert !Base.has_offset_axes(u, v, A)
if any(conv_with_offset, (axes(u)..., axes(v)..., axes(A)...))
throw(ArgumentError("offset axes not supported"))

Check warning on line 796 in src/dspbase.jl

View check run for this annotation

Codecov / codecov/patch

src/dspbase.jl#L796 

Added line #L796 was not covered by tests
end
m = length(u)+size(A,1)-1
n = length(v)+size(A,2)-1
B = zeros(T, m, n)
B[1:size(A,1),1:size(A,2)] = A
u = fft([u;zeros(T,m-length(u))])
v = fft([v;zeros(T,n-length(v))])
C = ifft(fft(B) .* (u * transpose(v)))
v = fft([v transpose(zeros(T,n-length(v)))])
C = ifft(fft(B) .* (u * v))
if T <: Real
return real(C)
end
Expand Down
5 changes: 3 additions & 2 deletions test/dsp.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,13 +198,14 @@ end
624 1388 1778 2082 2190 2298 2406 1638 688 280;
354 785 1001 1167 1221 1275 1329 903 379 154;
132 292 371 431 449 467 485 329 138 56]
@test_broken conv(u, v, A) == exp
@test_broken conv(u, transpose(v), A) == exp

fu = convert(Array{Float64}, u)
fv = convert(Array{Float64}, v)
fA = convert(Array{Float64}, A)
fexp = convert(Array{Float64}, exp)
@test conv(fu, fv, fA) ≈ fexp
@test @test_deprecated(conv(fu, fv, fA)) ≈ fexp
@test conv(fu, transpose(fv), fA) ≈ fexp

end

Expand Down
Loading