-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Extended the convolution filter for correct dilation #19303
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
Conversation
Changed the convolution filter from [1, 1] to [1, 1, 1] such that the mask is dilated on both sides, not only at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers
or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Thanks for the PR. I can imagine this is correct. Do you have some examples showing the previous version did bad things and this is better? |
Hi,
The following plots show the difference, they can be regeneratue using the
given example (https://matplotlib.org/gallery/event_handling/resample.html)
and setting ax.set_xlim
<https://matplotlib.org/api/_as_gen/matplotlib.axes.Axes.set_xlim.html#matplotlib.axes.Axes.set_xlim>(232,
234) instead of ax.set_xlim
<https://matplotlib.org/api/_as_gen/matplotlib.axes.Axes.set_xlim.html#matplotlib.axes.Axes.set_xlim>
(16, 365).
The first plot shows how it was before the PR and the second shows how it
will be after. As you can see, the first plot seems to start somewhere at
about 232.175, but actually the data is provided many timesteps before that
(from x-range = 16 to ..) Therefore, it makes sense that the line would
start before the first point that is shown in the first plot.
This is all the PR is about, the extended convolution filter makes sure
that the mask includes a point that lies before the shown x-range.
[image: 1_1.png]
[image: 1_1_1.png]
Thanks for reviewing and have a nice weekend,
Chris
Am Fr., 15. Jan. 2021 um 16:30 Uhr schrieb Jody Klymak <
notifications@github.com>:
… Thanks for the PR. I can imagine this is correct. Do you have some
examples showing the previous version did bad things and this is better?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#19303 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH5PD2VEBWT3YKPDDOTHDUDS2BNSRANCNFSM4WD4WNMA>
.
|
Your images didn't make it through via email. |
The following plots show the difference, they can be regeneratue using the given example (https://matplotlib.org/gallery/event_handling/resample.html) and setting ax.set_xlim(232, 234) instead of ax.set_xlim(16, 365). |
Makes sense to me. Thanks a lot! |
Changed the convolution filter from [1, 1] to [1, 1, 1] such that the mask is dilated on both sides, not only at the end.
PR Summary
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).