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

Make signal functions respect input dtype. #15366

Merged
merged 5 commits into from
Jan 14, 2022
Merged

Conversation

luigifcruz
Copy link
Contributor

This pull request ensures that the data type from the input and output are the same. This results in an expressive 35% speedup in single-precision Hilbert operations.

## Upstream
%timeit sc.hilbert(data64)
126 ms ± 284 us per loop (mean ‡ std. dev. of 7 runs, 10 Loops each)
%timeit sc.hilbert(data32)
101 ms ± 662 us per loop (mean ‡ std. dev. of 7 runs, 10 Loops each)
## Patch
In [5]: %timeit sc.hilbert (data64)
130 ms ± 259 us per loop (mean + std. dev. of 7 runs, 10 Loops each)
In [6]: %timeit sc.hilbert(data32)
75.6 ms ± 174 us per Loop (mean ‡ std. dev. of 7 runs, 10 Loops each)

@AtsushiSakai AtsushiSakai added enhancement A new feature or improvement scipy.signal labels Jan 6, 2022
@luigifcruz luigifcruz changed the title Make hilbert and hilbert2 respect input dtype. Make signal functions respect input dtype. Jan 12, 2022
@luigifcruz
Copy link
Contributor Author

I just noticed that the lfilter_zi function also doesn't respect the input type. The previous commit applies patches for this. A test for the function was also added. This commit might require re-review. cc @larsoner @ilayn

@luigifcruz
Copy link
Contributor Author

luigifcruz commented Jan 12, 2022

The CI is now throwing three test errors after the last commit. I can't reproduce them in my local machine. I'm not familiar with RuntimeWarning: invalid value encountered in ? (vectorized) error. I believe this happens only in 32-bits systems.

@luigifcruz
Copy link
Contributor Author

luigifcruz commented Jan 13, 2022

I tried to reproduce the errors reported by the CI with a 32-bits Debian VM but no luck. This error appears to be similar to this Satpy error. Any theories appreciated!

@larsoner
Copy link
Member

A restart fixed the errors, this LGTM, @tylerjereddy's comment has been addressed, and @ilayn approved already, so in it goes. Thanks @luigifcruz !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement scipy.signal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants