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.

Sign up for GitHub

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

Jump to bottom

add iir{notch,comb,peak} design functions #7634

Merged
merged 8 commits into from
Jul 20, 2023
Merged

add iir{notch,comb,peak} design functions #7634

merged 8 commits into from
Jul 20, 2023

Conversation

ev-br
Copy link
Contributor

@ev-br ev-br commented Jun 15, 2023 •
edited

cross-ref #7403

Port IIR filter design functions:

  • iircomb
  • iirnotch
  • iirpeak

This PR is on top of gh-7632 rebased.

@ev-br ev-br changed the title Iirnotch add iir{notch,comb,peak} functions Jun 15, 2023
@andfoy andfoy mentioned this pull request Jun 15, 2023
Open
@emcastillo emcastillo added cat:feature New features/APIs prio:medium labels Jun 16, 2023
@ev-br ev-br changed the title add iir{notch,comb,peak} functions add iir{notch,comb,peak, design} functions Jun 17, 2023
@ev-br
Copy link
Contributor Author

ev-br commented Jun 20, 2023 •
edited

rebased on gh-7632. Converting to draft until gh-7632 is decided on.

@ev-br ev-br marked this pull request as draft June 20, 2023 08:14
@mergify
Copy link
Contributor

mergify bot commented Jul 4, 2023

This pull request is now in conflicts. Could you fix it @ev-br? 🙏

@ev-br ev-br marked this pull request as ready for review July 12, 2023 14:08
@ev-br
Copy link
Contributor Author

ev-br commented Jul 12, 2023 •
edited

Rebased on main, now that gh-7632 is in. Ready for review from my side.

@takagi takagi changed the title add iir{notch,comb,peak, design} functions add iir{notch,comb,peak} design functions Jul 13, 2023
@takagi
Copy link
Member

takagi commented Jul 13, 2023

/test full

takagi
takagi reviewed Jul 13, 2023
View reviewed changes
cupyx/scipy/signal/_iir_filter_design.py

a = cupy.r_[1.0, -2.0 * gain * cupy.cos(w0), 2.0 * gain - 1.0]

return b, a
Copy link
Member

@takagi takagi Jul 13, 2023 •
edited

Choose a reason for hiding this comment

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

How about computing them on CPU and cupy.asarray just before return as the inputs are Python scalars and the data size is small?

takagi
takagi reviewed Jul 13, 2023
View reviewed changes
cupyx/scipy/signal/_iir_filter_design.py Outdated
G0, G = 1, 0
elif ftype == 'peak':
G0, G = 0, 1
GB = 1 / cupy.sqrt(2)
Copy link
Member

Choose a reason for hiding this comment

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

Computing on CPU, then setting values to a and b on GPU would be okay.

takagi
takagi reviewed Jul 13, 2023
View reviewed changes
cupyx/scipy/signal/_iir_filter_design.py Outdated
Comment on lines 756 to 757
beta = cupy.sqrt((GB**2 - G0**2) / (G**2 - GB**2)) * \
cupy.tan(N * w_delta / 4)
Copy link
Member

Choose a reason for hiding this comment

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

ditto (Computing on CPU)

@takagi
Copy link
Member

takagi commented Jul 13, 2023

Maybe older iircomb does not have pass_zero argument.

@ev-br
Copy link
Contributor Author

ev-br commented Jul 13, 2023

Indeed, it only appears in scipy 1.9.x. Will update the PR.

@ev-br
Copy link
Contributor Author

ev-br commented Jul 13, 2023

Pushed the updated PR: move scalar computations to CPU + guard a test for iirnotch(..., pass_zero=...) to only run with scipy >= 1.9.0.

takagi
takagi previously approved these changes Jul 14, 2023
View reviewed changes
Copy link
Member

@takagi takagi left a comment

Choose a reason for hiding this comment

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

LGTM!

@takagi
Copy link
Member

takagi commented Jul 14, 2023

/test full

@takagi takagi added this to the v13.0.0b1 milestone Jul 14, 2023
@takagi
Copy link
Member

takagi commented Jul 14, 2023

Would you check cupy.linux.cuda112 result, which uses scipy==1.7?

@ev-br
Copy link
Contributor Author

ev-br commented Jul 14, 2023 •
edited

Indeed, the failing test was checking a bug which was only fixed in scipy 1.9.x , too (scipy/scipy#16145). Added a commit to skip it.

@takagi
Copy link
Member

takagi commented Jul 20, 2023

/test full

@takagi
Copy link
Member

takagi commented Jul 20, 2023

Thanks! I'll merge this after CI finishes.

@takagi takagi merged commit aad9472 into cupy:main Jul 20, 2023
44 checks passed
@takagi
Copy link
Member

takagi commented Jul 20, 2023

Thanks, @ev-br!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@takagi takagi takagi approved these changes

Assignees

@takagi takagi

Labels
cat:feature New features/APIs prio:medium
Projects
Signal API (CZI EOSS 5)
Status: ✅ Done
Milestone
v13.0.0b1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ev-br @takagi @emcastillo