scatter and gather support element type of idx to be CartesianIndex #308
Conversation
|
One complaint that would be good to address is that NNlib should not be exporting gather and scatter. They are pretty generic names which may mean different things in different contexts. scatter for plotting - for ex |
|
unexporting gather and scatter is a breaking change, if we go with that the change should be done in another PR and the release marked as breaking |
|
However, scatter and gather are both essential low-level operations in computer science. These two operations here are to mimic the behavior of gather-scatter. |
|
Exporting them is a bug, which should come under semver. It isn't changing the behaviour of the functions itself. |
|
The fact that it interferes with Plots.jl means that many online tutorials of Flux are broken by exporting it (at least, if they use |
|
ok, I'm filing a PR removing the exports and tagging a patch release |
|
Ref the offending PR #255 |
|
@DhairyaLGandhi @CarloLucibello Is this ready to go? |
|
besides the slight simplification I suggested this seems ready to go |
|
Almost there, could add at least one test |
|
Seems like we can get rid of |
Co-authored-by: Carlo Lucibello <[email protected]> Update src/scatter.jl Co-authored-by: Carlo Lucibello <[email protected]> Update src/gather.jl Co-authored-by: Carlo Lucibello <[email protected]>
|
I noticed that I forgot to support |
| return dims | ||
| end | ||
|
|
||
| typelength(::Type{<:Number}) = 1 | ||
| typelength(::Type{<:NTuple{M}}) where M = M | ||
| function _check_dims(X::AbstractArray{Tx,Nx}, |
There was a problem hiding this comment.
Seems like we can unify the two methods with just converting the Cartesian indices once?
There was a problem hiding this comment.
these are very low-level methods, we should not do any allocation
No description provided.