Skip to content
This repository has been archived by the owner on Sep 25, 2023. It is now read-only.

Speedup: Single-precision hilbert, resample, and lfilter_zi. #447

Merged
merged 40 commits into from
Jan 18, 2022

Conversation

luigifcruz
Copy link
Contributor

Hi!

I noticed some functions weren't respecting the input type and thus returning double precision results.

This pull request proposes two changes:

  1. Make resample return a buffer with the same dtype as input.
  2. Add single-precision hilbert and hilbert2 kernels.

I also benchmarked each change:

  1. Upstream: 471 µs vs. Proposed: 260 µs
  2. Upstream: 1.47 ms vs. Proposed: 610 µs

raydouglass and others added 30 commits March 31, 2020 14:12
[HOTFIX] Fix missing six package on cusignal import [skip-ci-changelog]
[skip ci] Update master references for main branch
@luigifcruz luigifcruz requested a review from a team as a code owner January 6, 2022 03:57
@github-actions github-actions bot added the Python label Jan 6, 2022
@mnicely mnicely self-requested a review January 6, 2022 12:10
@mnicely
Copy link
Contributor

mnicely commented Jan 6, 2022

Hi @luigifcruz
Good catch on data type.
I'll get back to you soon with a review, I think there might be a simpler solution.

Copy link
Contributor

@mnicely mnicely left a comment

Choose a reason for hiding this comment

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

Can you please type specific tests as well, in test_filtering.py

    @pytest.mark.benchmark(group="Hilbert")
    @pytest.mark.parametrize("dtype", [np.float32, np.float64])
    @pytest.mark.parametrize("dim, num_samps", [(1, 2 ** 15), (2, 2 ** 8)])
    class TestHilbert:
...

python/cusignal/filtering/filtering.py Outdated Show resolved Hide resolved
python/cusignal/filtering/filtering.py Outdated Show resolved Hide resolved
python/cusignal/filtering/resample.py Show resolved Hide resolved
@awthomp awthomp added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 6, 2022
@awthomp awthomp changed the base branch from main to branch-22.02 January 6, 2022 13:35
@awthomp awthomp requested a review from a team as a code owner January 6, 2022 13:35
@awthomp
Copy link
Member

awthomp commented Jan 6, 2022

Thanks for the PR @luigifcruz! Once you make these suggested changes from @mnicely, we'll merge. I changed the merge target branch to branch-22.02 instead of main and added some CI flags to get all checks to clear. I still think we'll have a style change, but Matt may have pointed that out too.

Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

@luigifcruz luigifcruz changed the title Speedup: Single-precision hilbert and resample. Speedup: Single-precision hilbert, resample, and lfilter_zi. Jan 15, 2022
@luigifcruz
Copy link
Contributor Author

Thanks for the feedback @mnicely & @awthomp!

I implemented the requested style changes with 9d6d7aa and type check tests with bc9dcd9. I had to patch the linspace_data_gen function to accept a optional dtype argument with 279fe01. I also had to increase the tolerances of the array_equal method for single-precision arrays with 7ea86f6. This patch is required to successfully pass all tests. I believe this is caused by Scipy using double-precision arrays. I submitted a PR to Scipy but the fix should be available only with scipy>=1.9.0. You guys might want to handle the tolerances in a different way than I implemented.

I also noticed that the lfilter_zi method was returning a double-precision array no matter the input type. This would result in the filtfilt method always retuning a double-precision value. The patches for this are found in b5000ef.

@awthomp
Copy link
Member

awthomp commented Jan 18, 2022

@mnicely -- I don't think we can merge until you sign off on @luigifcruz 's fixes. Can you give this a look?

Looks great, Luigi. You're the best! Thanks, as always, for contributing.

@awthomp
Copy link
Member

awthomp commented Jan 18, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 80eadba into rapidsai:branch-22.02 Jan 18, 2022
@luigifcruz
Copy link
Contributor Author

Awesome, @awthomp! Thanks all for maintaining this awesome project!

@luigifcruz luigifcruz deleted the main branch January 18, 2022 18:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants