Add split shape utility #3062
Merged
Add split shape utility #3062
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jantonguirao
force-pushed
the
split_shape
branch
from
17f7922
to
294ee75
Compare
mzient
reviewed
Jun 17, 2021
dali/kernels/common/split_shape.h
Outdated
| /** | ||
| * @brief Utility to divide a bigger shape into smaller blocks, given a desired number of blocks | ||
| * and a minimum practical block size. | ||
| * The algorithm start splitting from the outtermost dimension until the number of blocks |
There was a problem hiding this comment.
Suggested change
| * The algorithm start splitting from the outtermost dimension until the number of blocks | |
| * The algorithm starts splitting from the outermost dimension until the number of blocks |
mzient
reviewed
Jun 17, 2021
Comment on lines
+31
to
+33
| template <typename SplitFactor, typename Shape> | ||
| void split_shape(SplitFactor& split_factor, const Shape& in_shape, int min_nblocks = 8, | ||
| int min_sz = (16 << 10)) { |
There was a problem hiding this comment.
If this is intended for threading, then there are several issues:
- If we can divide the shape into a number of threads that's a multiple of thread count, then we need no more partitions.
- We should avoid uneven splits unless there's a good chance that it will be masked by the large number of blocks (much more than threads).
There was a problem hiding this comment.
My idea is that the caller will specify a suitable value for min_nblocks (a multiple of the number of threads)
jantonguirao
force-pushed
the
split_shape
branch
from
294ee75
to
9676f7c
Compare
Signed-off-by: Joaquin Anton <janton@nvidia.com>
jantonguirao
force-pushed
the
split_shape
branch
from
9676f7c
to
b57f9e4
Compare
mzient
reviewed
Jun 17, 2021
dali/kernels/common/split_shape.h
Outdated
Comment on lines
34
to
35
| int ndim = in_shape.size(); | ||
| assert(static_cast<int>(split_factor.size()) == ndim); |
There was a problem hiding this comment.
Suggested change
| int ndim = in_shape.size(); | |
| assert(static_cast<int>(split_factor.size()) == ndim); | |
| int ndim = dali::size(in_shape); | |
| assert(static_cast<int>(dali::size(size) == ndim); |
mzient
reviewed
Jun 17, 2021
dali/kernels/common/split_shape.h
Outdated
| for (int d = 0; d < ndim; d++) | ||
| split_factor[d] = 1; | ||
|
|
||
| int64_t vol = volume(in_shape.begin(), in_shape.end()); |
There was a problem hiding this comment.
If it's iterable, volume should know what to do.
Suggested change
| int64_t vol = volume(in_shape.begin(), in_shape.end()); | |
| int64_t vol = volume(in_shape); |
jantonguirao
force-pushed
the
split_shape
branch
from
aa6041b
to
648d04d
Compare
mzient
reviewed
Jun 17, 2021
| TensorShape<> sh(10, 10, 10); | ||
| std::vector<int> split_factor = {1, 1, 1}; | ||
|
|
||
| split_shape(split_factor, sh, 3, 1000); // minimum volume is bigger than the input volume |
There was a problem hiding this comment.
Nitpick ;)
Suggested change
| split_shape(split_factor, sh, 3, 1000); // minimum volume is bigger than the input volume | |
| split_shape(split_factor, sh, 3, 1000); // minimum volume is equal than the input volume |
Signed-off-by: Joaquin Anton <janton@nvidia.com>
jantonguirao
force-pushed
the
split_shape
branch
from
648d04d
to
66717ff
Compare
mzient
approved these changes
Jun 17, 2021
banasraf
approved these changes
Jun 17, 2021
|
!build |
|
CI MESSAGE: [2484608]: BUILD STARTED |
|
CI MESSAGE: [2484608]: BUILD PASSED |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Joaquin Anton janton@nvidia.com
Why we need this PR?
Pick one, remove the rest
What happened in this PR?
Fill relevant points, put NA otherwise. Replace anything inside []
Introduced a utility function that provides per-dimension split factor given a shape, a desired number of blocks and a minimum practical block size.
NA
split_shape implementation, tests (did I miss any meaningful testcase?)
Unit tested
Doxygen
JIRA TASK: [DALI-2128]