-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix broadcast_indices #22130
Fix broadcast_indices #22130
Conversation
This fixes a regression introduced in 4f1b479. broadcast_indices() needs to be overloaded by packages for custom types, so it cannot be hidden under _broadcast_indices(). Also, ::Type is incorrect since the method only applies to scalars. Make the tests more complex to be closer to actual implementations in packages so that regressions like this will be noticed in the future.
cc74744
to
8123644
Compare
I just realized we already have |
Yes, underscored and not exported usually means the exact opposite of public API. |
Good to go? |
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.
lgtm! :)
Thanks. It would still be interesting to have @vtjnash double-check this, in case we missed something. |
Sorry, I missed the review request. |
No worries, four people considered it was a reasonable change, it passed the tests, and it fixes DataArrays tests. So the burden of the proof is on Jameson! ;-) |
you might need better tests... :P (yes, I realize this is also my fault for not adding a test when I fixed this code) This PR broke cases like the following:
(causes them to compute the wrong size, shape, and type) |
This reverts commit fb81c34. And adds a test for the bug this causes.
This reverts commit fb81c34. And adds a test for the bug this causes.
This fixes a regression introduced in 4f1b479.
broadcast_indices()
needs tobe overloaded by packages for custom types, so it cannot be hidden under
_broadcast_indices()
. Also,::Type
is incorrect since the method only appliesto scalars.
Make the tests more complex to be closer to actual implementations in packages
so that regressions like this will be noticed in the future.
Fixes a bug in DataArrays: JuliaStats/DataArrays.jl#259 (comment)