-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add boundary='wrap'
support for jax.scipy.signal.convolve2d
and jax.scipy.signal.correlate2d
#21241
base: main
Are you sure you want to change the base?
Conversation
Looks great! The last thing we need is to squash the changes into a single commit (see https://jax.readthedocs.io/en/latest/contributing.html#single-change-commits-and-pull-requests). Let me know if you need help with that process |
2ec134e
to
5760dc2
Compare
Squashed all the commits into a single one. |
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.
Looks great, thanks!
We're seeing errors when running a larger number of test cases. You can reproduce them locally using e.g.
It looks like it's an issue with |
Example failure:
|
It is fixed now. It is the problem with padding |
…cipy.signal.correlate2d
8035ab5
to
5773c5b
Compare
That's surprising to me – say you are convolving two Can we fix this without blowing up the memory requirements in cases like this? |
I will check and let you know, if it is possible or not. |
I could not find any other way to fix the issue without padding it with maximum value in all dimensions. |
I would consider this a blocker to merging this PR: we need to figure out how to implement this without excessive padding. |
Currently
jax.scipy.signal.convolve2d
andjax.scipy.signal.correlate2d
functions support onlyboundary='fill'
. This PR will addboundary='wrap'
support for both the functions.Fixes #7276