Implement broadcast_arrays, broadcast_to
#1020
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1020 +/- ##
==========================================
- Coverage 91.85% 91.80% -0.06%
==========================================
Files 74 72 -2
Lines 10712 10514 -198
==========================================
- Hits 9840 9652 -188
+ Misses 872 862 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Thanks a lot @neosunhan and apologies again for the Draft PR mix-up.
These two functions don't return the expected values when the input arrays are distributed. I've suggested a change for broadcast_to that leverages all the broadcasting checks in _operations.__bin_op().
You should be able to reimplement broadcast_arrays using the new version of broadcast_to so that distributed arrays can be broadcasted even if they are distributed along different axes, e.g. this should be possible:
a = ht.arange(100, split=0)
b = ht.arange(200).reshape(2,100)
b.resplit_(axis=1)
# a.split is 0, b.split is 1
ht.broadcast_arrays(a,b)Thanks a lot!
for more information, see https://pre-commit.ci
…tics/heat into feature/778-broadcasting
|
Thank you for the PR! |
|
Thank you for the PR! |
There was a problem hiding this comment.
Review for PR #1020 "Implement broadcast_arrays, broadcast_to"
This PR adds two broadcasting-routines known from NumPy to Heat.
As far as I can judge, the code is correct and sufficiently commented. However, I would like to suggest to add some more information into the documentation, either by an example what each of the two routines does for a concrete array or by providing reference to the corresponding NumPy-functionality.
Once these changes have been done, I recommend merging.
|
Thank you for the PR! |
|
Thank you for the PR! |
|
Thank you for the PR! |
|
Thank you for the PR! |
|
Thank you for the PR! |
|
Thank you for the PR! |
mrfh92
dismissed
ClaudiaComito’s stale review
outdated comment from Feb 09 is still blocking merge
Description
Implement
broadcast_arrays,broadcast_toIssue/s resolved: #778
Changes proposed:
Type of change
Due Diligence
Does this change modify the behaviour of other functions? If so, which?
no
skip ci