search

JuliaDSP / DSP.jl

Filter design, periodograms, window functions, and other digital signal processing functionality
https://docs.juliadsp.org/stable/contents/
Other
381 stars 110 forks source link

conv2 int and float #263

Closed HDictus closed 5 years ago

HDictus commented 5 years ago

Running conv2 on arrays where one is an integer array and one is a float array results in an error. As multiplication between int and float is defined, I see no reason to restrict conv/conv2 in this respect, and It violates the julia style guide's "Avoid writing overly specific types". In principle, any two arrays whose elements can be multiplied by one another can be convolved, so I think it is best to require only this of the arrays (hence duck typing them).

This way the user can also use conv/conv2d to convolve user-defined types as long as they have defined a multiplication operation for them.

I'd be happy to implement it myself. I'm pretty new to Julia though so my first attempt will probably require some correction by a veteran.

galenlynch commented 5 years ago

If you're willing to put together a PR it would be welcome. I just tried the obvious solution, adding a new non-parametric method that promotes its inputs, but was having some problems with the StridedVector type alias without type parameters. I also noticed that conv2 has no tests. Did the functions in dspbase get moved into DSP.jl from base without their tests?

galenlynch commented 5 years ago

Just a word of guidance for your PR: you should just try to get the inputs to have a common element type, and then let existing methods do the rest. conv2 does not use the straight-forward multiplication algorithm for convoultion, but instead uses fourier methods for better performance.

HDictus commented 5 years ago

Thanks! I checked the commit for julia 0.6.4 julia/test/dsp.jl , and there too there are no tests for conv2. I may as well implement some tests while I'm at it, I just hope that whatever I do to conv2 doesn't affect functionality in ways other than I expect, as without preexisting tests behavior I don't think to test before and after may change. Edit: conv, dconv and xcorr in dspbase do have tests, in test/dsp.jl. It's just conv2 that's missing them.

HDictus commented 5 years ago

I've decided to tackle #202 before this, as it is also a feature I would like to see, and it would be easier to do them in that order.

galenlynch commented 5 years ago

Implemented by #269.