split_v2 operator #13687
split_v2 operator #13687
Conversation
|
@HyperZealot Thanks for working on this. |
|
Could you explain the difference between split(sliceChannel) and split_v2 , and maybe show some performance compare between these two OP? Thanks |
HyperZealot
force-pushed
the
split_v2
branch
from
35447ff
to
fc35119
Compare
| indices = [0] + list(indices_or_sections) | ||
| else: | ||
| raise ValueError('indices_or_sections must either int or tuple of ints') | ||
| return _internal._split_v2(ary, indices, axis, squeeze_axis, sections) |
There was a problem hiding this comment.
why the symbol implementation and ndarray implementation are different? the ndarray impl always computes indices and here it may pass the number of sections directly.
There was a problem hiding this comment.
You do not know the shape of a symbol so you cannot pre-compute the indices in symbol mode.
| } | ||
| }; // struct SplitParam | ||
|
|
||
| inline TShape GetSplitIndices(const TShape& ishape, int axis, int sections) { |
There was a problem hiding this comment.
maybe return Tuple may make more sense.
There was a problem hiding this comment.
TShape is also a Tuple: https://github.com/dmlc/tvm/blob/master/nnvm/include/nnvm/tuple.h#L325. But maybe I can also switch to nnvm::Tuple.
There was a problem hiding this comment.
i know TShape uses Tuple. I think it's better to use Tuple for the return result. TShape makes the return result look like a shape.
|
@apeforest Please also avoid v2 of the same question as well, I think ur question was asked by @rongzha1 already. |
|
@zheng-da Can you take a look again to see if your comments are addressed? Thanks |
| } | ||
| }; // struct SplitParam | ||
|
|
||
| inline TShape GetSplitIndices(const TShape& ishape, int axis, int sections) { |
There was a problem hiding this comment.
i know TShape uses Tuple. I think it's better to use Tuple for the return result. TShape makes the return result look like a shape.
| const size_t section_size = indices[target + 1] - indices[target]; | ||
| const size_t target_idx = | ||
| head_idx * trailing_size * section_size + mid_idx * trailing_size + tail_idx; | ||
| target_data[target_idx] = in_data[i]; |
There was a problem hiding this comment.
I'm a little concerned about this kernel. It takes a lot of computation to copy an element from the original array to a destination array. At least we should copy the entire row unless we split in the last dimension.
There was a problem hiding this comment.
I doubt if memcpy is optimal for all cases actually, and splitting the kernel into 2 versions would also make the code harder to maintain and manage, what do you think if we treat that as a further optimization and do it in a follow-up PR so that the users of DGL can enjoy this new op ASAP?
Description
New version of split operator to match the behavior of numpy.split
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments