Support batch 1-d convolution in ht.signal.convolve
#1515
Conversation
|
Thank you for the PR! |
|
Thank you for the PR! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1515 +/- ##
==========================================
+ Coverage 91.85% 91.89% +0.03%
==========================================
Files 80 80
Lines 11878 11916 +38
==========================================
+ Hits 10911 10950 +39
+ Misses 967 966 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Thank you for the PR! |
ClaudiaComito
requested review from
mrfh92 and
JuanPedroGHM
and removed request for
mrfh92
|
Thank you for the PR! |
| v.resplit_(axis=None) | ||
| else: | ||
| v.resplit_(axis=a.split) | ||
| batch_processing = True |
There was a problem hiding this comment.
Why is batch_processing = True hard coded?
There was a problem hiding this comment.
Why is batch_processing = True hard coded?
Hi @krajsek , do you mean, it would be better to let the user set it as a keyword argument? If so, I agree.
There was a problem hiding this comment.
As far as I understand things, it is set batch_processing=True and, depending on some conditions, it is set batch_processing=False lateron; in my opinion this is fine and I would not suggest to introduce a kwarg since actually the correct value of batch_processing is already uniquely determined by the other inputs.
There was a problem hiding this comment.
o.k., I agree, I also do not advocate for kwarg. Setting variables to some values in code always irritates me, but after reading the code again it makes sense. Maybe we spend a comment why this is hard coded here.
|
Thank you for the PR! |
|
@JuanPedroGHM @krajsek @mrfh92 thanks for reviewing, I think I addressed all points, let me know if something's missing. |
ClaudiaComito
dismissed
JuanPedroGHM’s stale review
All change requests addressed, thanks JP!
Due Diligence
Description
This PR expands
ht.signal.convolveto perform 1-D convolutions on a batch of 1-D arrays, i.e.if
signal.shape = (..., n)andkernel.shape = (m,):signal's last dimension will be convolved withkernel.if e.g.
signal.shape = (i, j, n)andkernel.shape = (i, j, m,): each elementsignal[i, j]of sizenwill be convolved with the corresponding elementkernel[i,j]of sizem.Signal and kernel can be any number of dimensions, as long as the batch dimensions match.
Signal and kernel can be distributed along any of the batch dimensions.
Issue/s resolved: #1514
Related: #1396
Changes proposed:
Type of change
Memory requirements
Performance
Does this change modify the behaviour of other functions? If so, which?
yes, instead of returning
ValueErrorwhensignalis n-D,ht.convolvenow raises a ValueError only if batch convolution isn't possible (shapes mismatch, unsuitable distribution)