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

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

Merged
merged 3 commits into from
Nov 11, 2024

Conversation

martinholters
Copy link
Member

Prepare for a possible future multi-arg conv (cf. #338, #224, #166) by deprecating the current conv(u::AbstractVector, v::AbstractVector, A::AbstractMatrix) (which would have a conflicting meaning) to conv(u::AbstractVector, v::Transpose{T,<:AbstractVector}, A::AbstractMatrix) which is consistent with a future conv(u, v, A) == conv(conv(u, v), A)), just more effcient.

See #224 (comment), #166 (comment).

Even without the general n-arg conv, I think this signature makes more sense as it makes it explicit which of u and v operates in which direction. (Which before wasn't even documented.)

@martinholters martinholters added this to the 0.8 milestone Nov 5, 2024
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.84%. Comparing base (1e30c9a) to head (bf14a3c).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/dspbase.jl 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #577      +/-   ##
==========================================
- Coverage   97.87%   97.84%   -0.04%     
==========================================
  Files          19       19              
  Lines        3251     3252       +1     
==========================================
  Hits         3182     3182              
- Misses         69       70       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/dspbase.jl Outdated Show resolved Hide resolved
@martinholters martinholters force-pushed the mh/three-arg-conv-deprecation branch 2 times, most recently from b0969dc to b24091f Compare November 5, 2024 11:08
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.

@martinholters
Copy link
Member Author

Another pair of eyes would be welcome, but absent objections, I plan to merge this soonish.

@martinholters martinholters merged commit 3c1b783 into master Nov 11, 2024
9 of 11 checks passed
@martinholters martinholters deleted the mh/three-arg-conv-deprecation branch November 11, 2024 07:40
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