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? |
|
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,::Typeis 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)