Utils and prerequisities for NppRemapKernel implementation
#4374
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
Signed-off-by: szalpal <mszolucha@nvidia.com>
|
!build |
|
CI MESSAGE: [6235052]: BUILD STARTED |
18 tasks
szalpal
commented
Oct 19, 2022
Comment on lines
+484
to
+499
| /** @{ */ | ||
| /** @brief Construction from contiguous memory */ | ||
|
|
||
| TensorListView(std::nullptr_t, const std::vector<TensorShape<DynamicDimensions>> &shapes) | ||
| : Base(TensorListShape<DynamicDimensions>(shapes)) {} | ||
|
|
||
| template <int other_sample_ndim> | ||
| TensorListView(std::nullptr_t, const TensorListShape<other_sample_ndim> &shape) | ||
| : Base(shape) {} | ||
|
|
||
| template <int other_sample_ndim> | ||
| TensorListView(std::nullptr_t, TensorListShape<other_sample_ndim> &&shape) | ||
| : Base(std::move(shape)) {} | ||
|
|
||
| /** @} */ | ||
|
|
There was a problem hiding this comment.
These constructors already exist in static_dim flavour of TensorListView. Here I'm adding them to DynamicDimensions flavour as well.
|
CI MESSAGE: [6235052]: BUILD PASSED |
banasraf
reviewed
Oct 20, 2022
Comment on lines
+142
to
+146
| auto ridx = ndims; | ||
| for (size_t idx = 0; idx < ndims; idx++) { | ||
| ret[--ridx] = e[idx]; | ||
| } | ||
| return ret; |
There was a problem hiding this comment.
You want to reverse the order here, right?
Suggested change
| auto ridx = ndims; | |
| for (size_t idx = 0; idx < ndims; idx++) { | |
| ret[--ridx] = e[idx]; | |
| } | |
| return ret; | |
| std::reverse(e); | |
| return TensorShape<ndims>(e.begin(), e.end()); |
this should work, I guess
There was a problem hiding this comment.
maybe better:
TensorShape<ndim> ret;
std::reverse_copy(e.begin(), e.end(), ret.begin());
return ret;There was a problem hiding this comment.
Good point. Followed up in #4375
banasraf
approved these changes
Oct 20, 2022
awolant
approved these changes
Oct 20, 2022
| namespace detail { | ||
|
|
||
| template<typename T, typename Tuple> | ||
| struct contains; |
There was a problem hiding this comment.
Tbh. I liked the old name has_type better but it is just a matter of preference, I guess. I know this name was suggested by other reviewer so no point in changing it back and forth :)
18 tasks
Closed
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: szalpal mszolucha@nvidia.com
Category:
New feature (non-breaking change which adds functionality)
Refactoring (Redesign of existing code that doesn't affect functionality)
Other (e.g. Documentation, Tests, Configuration)
Description:
Since the #4365 PR grew significantly in size, I decided to extract parts of it into the separate PR to enhance review quality. This PR includes some utilities I've used for
NppRemapKernelimplementation. Specifically:make_tensor_listfunctions: creatingTensorListViewfrom aTensorView(just wrapping it) and formvector<TensorView>. The latter overload is possible, since we changed from contiguous memory pattern to non-contiguous one. The tests are included intensor_view_test.cc.contains_v- convenient trait, that checks if a tuple has a given type. Surprisingly, such thing doesn't exist instd...tensor_to_matfunction. It doesn't needStorageCPU. It just needs, that the memory is CPU-accessible.ShapeFromRoithat returnTensorShapethat can be associated with givenRoi. The functions that are already there don't cover the case, when there is no channel dimension. And such situation might happen for 1-channel images - channel dimension doesn't have to exist.Additional information:
Affected modules and functionalities:
Key points relevant for the review:
Tests:
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A