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

exchange BLAS.BlasFloat for Union{AbstractFloat, Complex{<:AbstractFl… #477

Merged
merged 4 commits into from
Aug 23, 2022

Conversation

MikaelSlevinsky
Copy link
Contributor

@MikaelSlevinsky MikaelSlevinsky commented Jul 20, 2022

…oat}}

There is an upcoming package for FFTs on generic number types https://github.com/JuliaApproximation/GenericFFT.jl. This will enable, in particular, convolutions with BigFloat and Complex{BigFloat} arrays. To make this Just WorkTM, we may simply remove the type restrictions referring to FFTW number types (quoted as BLAS.BlasFloats). Then when both packages are loaded, using DSP, GenericFFT, conv(rand(BigFloat, 10), rand(BigFloat, 20)) works analogously to finding BigFloat eigenvalues through GenericLinearAlgebra.

@MikaelSlevinsky
Copy link
Contributor Author

Hi there, the tests pass locally for me and the change seems rather benign overall. Could a maintainer please take a look and approve running workflows?

@MikaelSlevinsky
Copy link
Contributor Author

92/93! Yahoo! (I think that coverage is actually cumulative, so I'm not sure it's true that total coverage decreased.)

@MikaelSlevinsky
Copy link
Contributor Author

Ping

MikaelSlevinsky added a commit to JuliaApproximation/FastTransforms.jl that referenced this pull request Aug 3, 2022
preserve `conv` pirating until DSP's PR gets merged and tagged JuliaDSP/DSP.jl#477
@MikaelSlevinsky
Copy link
Contributor Author

Hello, could a maintainer please take a look at this? Thank you

@galenlynch
Copy link
Member

Yes, I'll look soon, sorry!

src/dspbase.jl Outdated Show resolved Hide resolved
Co-authored-by: Dehann Fourie <6412556+dehann@users.noreply.github.com>
@MikaelSlevinsky
Copy link
Contributor Author

Any other changes requested?

src/dspbase.jl Outdated Show resolved Hide resolved
@martinholters
Copy link
Member

This should probably be ok. But once we get to addressing #292, it would be unclear which types to use frequency domain or time domain convolution for if the applicability of fft depends on whether some other package (GenericFFT) has been loaded. Given how long #292 has been open, that shouldn't stop this PR offering an immediate improvement for now, though.

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

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

I think this is fine. Any comments, @galenlynch? Merge and tag?

@MikaelSlevinsky
Copy link
Contributor Author

This can probably be merged, right? FYI, this benign change reduces my package load time by about 40%.

@martinholters martinholters merged commit 13265f5 into JuliaDSP:master Aug 23, 2022
@galenlynch
Copy link
Member

Sorry I missed this, @martinholters. I'll try to finish #292. I got stuck figuring out how to best select between the many conv methods after implementing a bunch of different variants that work better in different situations. I think for now it may be better to accept occasionally choosing the wrong method in the name of being faster and more general, on average.

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.

None yet

4 participants