Skip to content

1D Convolution in Julia - convolve_cyclic not commutative #853

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

Closed
4 tasks
jiegillet opened this issue Aug 31, 2021 · 1 comment
Closed
4 tasks

1D Convolution in Julia - convolve_cyclic not commutative #853

jiegillet opened this issue Aug 31, 2021 · 1 comment
Labels
lang: julia Julia programming language Problem This is a problem in the archive or an implementation.

Comments

@jiegillet
Copy link
Member

Bug Report

Description

The convolution operation should be commutative, but the Julia implementation isn't for inputs of different size.

Steps to Reproduce

julia> convolve_cyclic([3, 4, 5], [1, 2])
3-element Array{Float64,1}:
 13.0
 13.0
 10.0

julia> convolve_cyclic([1, 2], [3, 4, 5])
ERROR: BoundsError: attempt to access 2-element Array{Int64,1} at index [3]
Stacktrace:
 [1] getindex at ./array.jl:809 [inlined]
 [2] convolve_cyclic(::Array{Int64,1}, ::Array{Int64,1}) at /Users/jie/Documents/algorithm-archive/contents/convolutions/code/julia/1d_convolution.jl:17
 [3] top-level scope at REPL[52]:1

When the signal is larger, the filter is padded with 0s (conceptually, maybe not in the implementation). It should go the other way around too. The following lines even suggest that:

# output size will be the size of sign
output_size = max(length(signal), length(filter))

Expected behavior

julia> convolve_cyclic([3, 4, 5], [1, 2])
3-element Array{Float64,1}:
 13.0
 13.0
 10.0

julia> convolve_cyclic([1, 2], [3, 4, 5])
 13.0
 13.0
 10.0

For Algorithm Archive Developers

  • The bug can be reproduced
  • The bug can be fixed (if not, please explain why not in a comment below)
  • There is a timeline to fix the bug
  • The bug has been fixed (Please link the PR)
@jiegillet jiegillet added Problem This is a problem in the archive or an implementation. lang: julia Julia programming language labels Aug 31, 2021
jiegillet added a commit to jiegillet/algorithm-archive that referenced this issue Aug 31, 2021
leios added a commit that referenced this issue Aug 31, 2021
* [#852] [#853] Fix for Julia 1D convolution

* fixing line numbers for jie's PR

Co-authored-by: James Schloss <jrs.schloss@gmail.com>
@leios
Copy link
Member

leios commented Aug 31, 2021

I believe this was fixed by #854

@leios leios closed this as completed Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang: julia Julia programming language Problem This is a problem in the archive or an implementation.
Projects
None yet
Development

No branches or pull requests

2 participants