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

for loop fix to follow math formula #125

Merged
merged 4 commits into from
May 5, 2020
Merged

Conversation

elfelround
Copy link
Contributor

for loop not following mathematical formula

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
for loop not following mathematical formula
@elfelround elfelround changed the title Update ch15_part1.py for loop fix to follo math formula Apr 28, 2020
@elfelround elfelround changed the title for loop fix to follo math formula for loop fix to follow math formula Apr 28, 2020
@vmirly
Copy link
Collaborator

vmirly commented Apr 28, 2020

Hi @elfelround ,

thanks for the suggestion. I agree with your suggestion, it is more general. However, I think the division by s should be (len(x_padded) - len(w_rot) ) / s so in the suggested change, one parentheses is missing.

In this function, the assumption is that we are using "same" padding, that's why we also compare with np.convolve(x, w, mode='same'). The current formulation in this function would work for any combination of kernel-size and padding-size that results in "same"-padding, for example if kernel has size 3, and padding p=1, the function would still work.

I will discuss it with Sebastian for this change. I appreciate your suggestion. 👍 :

@vmirly vmirly requested a review from rasbt April 28, 2020 21:07

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@pep8speaks
Copy link

pep8speaks commented Apr 29, 2020

Hello @elfelround! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-05-05 16:56:28 UTC

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@elfelround
Copy link
Contributor Author

@pep8speaks now

@rasbt
Copy link
Owner

rasbt commented May 5, 2020

Thanks a lot for the PR. I agree with Vahid, your suggestions is closer to the math formula in the book and the more general solution. While the current one (for i in range(0, int(len(x)/s), s)) works here, it wouldn't work for settings where p=0, for example. Thanks for correcting that!

@rasbt rasbt merged commit f4bb301 into rasbt:master May 5, 2020
@elfelround elfelround deleted the patch-3 branch May 5, 2020 23:04
@elfelround
Copy link
Contributor Author

elfelround commented May 24, 2020

submitted a packt digital credit request for a pdf upon this and the pickle issue today as erick lestrange. (im unsure if i already had a credit for this book but i think not) cheers @rasbt

@rasbt
Copy link
Owner

rasbt commented May 26, 2020

Just wanted to say thanks again for all the feedback. You have a great eye for detail. We really appreciate it!

@elfelround
Copy link
Contributor Author

elfelround commented May 31, 2020

@rasbt i have more errata but they are mainly irrelevant typos or refractorings. crushing on you and your book was really good, enjoyed every bit of it, ure welcome to add me on linkedin if in the future (or not, both are fine) i have enough lecture for ages but ill be looking after any of ur published work in the future
and thank you so much for your work, i find your book is a great contribution for society~

linkedin.com/in/ericklestrange/
xx

@rasbt
Copy link
Owner

rasbt commented Jun 9, 2020

Thanks so much for the kind words! And I am glad to hear that you have a good experience with the book overall! If it is not too much effort to type them up, I'd be great to get a list of these typos. With traditional publishing, it's hard to get things updated retrospectively, but it would be useful in case we are planning a 4th edition one day.

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