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

[DOC] Why devide the sum(signal) in the 'cusignal/notebooks/api_guide/convolution_examples.ipynb' ? #303

Closed
vBaiCai opened this issue Jan 10, 2021 · 2 comments · Fixed by #310
Assignees
Labels
1 - On Deck To be worked on next doc Documentation
Milestone

Comments

@vBaiCai
Copy link

vBaiCai commented Jan 10, 2021

Report incorrect documentation

Location of incorrect documentation
cusignal/notebooks/api_guide/convolution_examples.ipynb
in convolution section.

Describe the problems or issues found in the documentation

cconv = signal.convolve(sig, win, mode='same') / np.sum(sig)

Steps taken to verify documentation is incorrect

Suggested fix for documentation

I think, the np.sum(sig) may be a typo. In general,we normalize the convolved signal using sum(win) like this. I can’t figure out why you devide the sum(sig) here.

Report needed documentation

Report needed documentation

Describe the documentation you'd like
cconv = signal.convolve(sig, win, mode='same') / np.sum(win)

Steps taken to search for needed documentation

@vBaiCai vBaiCai added ? - Needs Triage Need team to review and classify doc Documentation labels Jan 10, 2021
@awthomp
Copy link
Member

awthomp commented Jan 11, 2021

Hey @vBaiCai -- thanks for the DOC report on cuSignal!

You're totally right: this is supposed to be np.sum(win) for the CPU portion and cp.sum(win) part. Looks like we have a typo in the CPU section (e.g using np.sum(sig) instead of np.sum(win)). Would you be willing to submit a PR? I am more than happy to guide you through the process!

@awthomp awthomp added 1 - On Deck To be worked on next and removed ? - Needs Triage Need team to review and classify labels Jan 11, 2021
@awthomp awthomp added this to the 0.18 milestone Jan 11, 2021
@awthomp awthomp self-assigned this Jan 11, 2021
@vBaiCai
Copy link
Author

vBaiCai commented Jan 11, 2021

Yes, I'm willing to submit a PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 - On Deck To be worked on next doc Documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants