Adding support for aten::topk #302
Conversation
github-actions
bot
added
component: conversion
| auto k = args[1].unwrapToInt(); | ||
| auto dim = args[2].unwrapToInt(); | ||
| auto largest = args[3].unwrapToBool(); | ||
| // auto sorted = args[4].unwrapToBool(); # Currently unused |
There was a problem hiding this comment.
@inocsin What is the role of this argument and why don't we use it? I commented it out because it was throwing warnings
There was a problem hiding this comment.
torch.topk has an argument sorted = True which returns the top k elements in a sorted order (decreasing). By default it is true. Setting it False will result in random order of output top k elements which we can't support in TensorRT. TensorRT always returns the top "k" max or min elements depending on "largest" argument. Maybe we can add this detail as a comment to explain why we aren't using args[4]
There was a problem hiding this comment.
will this cause an issue to users? if they expect random does always giving them ordered matter? At the very least yeah there should be a comment/
There was a problem hiding this comment.
This won't cause an issue to users in my opinion. So far all usecases for this op have been gathering either top k largest elements or top k smallest elements, both of which are supported. We can revisit this if anyone requests such usecase.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
There are some changes that do not conform to C++ style guidelines:
diff --git a/workspace/core/conversion/converters/impl/topk.cpp b/tmp/changes.txt
index 2addbdc..d052dd9 100644
--- a/workspace/core/conversion/converters/impl/topk.cpp
+++ b/tmp/changes.txt
@@ -25,11 +25,12 @@ auto topk_registrations TRTORCH_UNUSED = RegisterNodeConversionPatterns().patter
auto selfDim = util::toVec(self->getDimensions());
- //reduceAxes The reduction dimensions. The bit in position i of bitmask reduceAxes corresponds to explicit dimension i of the result.
- //E.g., the least significant bit corresponds to the first explicit dimension and the next to least significant bit corresponds to the second explicit dimension.
+ // reduceAxes The reduction dimensions. The bit in position i of bitmask reduceAxes corresponds to explicit
+ // dimension i of the result. E.g., the least significant bit corresponds to the first explicit dimension and the
+ // next to least significant bit corresponds to the second explicit dimension.
if (dim < 0) {
- dim = selfDim.size() + dim;
+ dim = selfDim.size() + dim;
}
uint32_t shiftDim = 1 << dim;
ERROR: Some files do not conform to style guidelines
There was a problem hiding this comment.
There are some changes that do not conform to C++ style guidelines:
diff --git a/workspace/core/conversion/converters/impl/topk.cpp b/tmp/changes.txt
index 2addbdc..d052dd9 100644
--- a/workspace/core/conversion/converters/impl/topk.cpp
+++ b/tmp/changes.txt
@@ -25,11 +25,12 @@ auto topk_registrations TRTORCH_UNUSED = RegisterNodeConversionPatterns().patter
auto selfDim = util::toVec(self->getDimensions());
- //reduceAxes The reduction dimensions. The bit in position i of bitmask reduceAxes corresponds to explicit dimension i of the result.
- //E.g., the least significant bit corresponds to the first explicit dimension and the next to least significant bit corresponds to the second explicit dimension.
+ // reduceAxes The reduction dimensions. The bit in position i of bitmask reduceAxes corresponds to explicit
+ // dimension i of the result. E.g., the least significant bit corresponds to the first explicit dimension and the
+ // next to least significant bit corresponds to the second explicit dimension.
if (dim < 0) {
- dim = selfDim.size() + dim;
+ dim = selfDim.size() + dim;
}
uint32_t shiftDim = 1 << dim;
ERROR: Some files do not conform to style guidelines
| auto k = args[1].unwrapToInt(); | ||
| auto dim = args[2].unwrapToInt(); | ||
| auto largest = args[3].unwrapToBool(); | ||
| // auto sorted = args[4].unwrapToBool(); # Currently unused |
There was a problem hiding this comment.
torch.topk has an argument sorted = True which returns the top k elements in a sorted order (decreasing). By default it is true. Setting it False will result in random order of output top k elements which we can't support in TensorRT. TensorRT always returns the top "k" max or min elements depending on "largest" argument. Maybe we can add this detail as a comment to explain why we aren't using args[4]
Signed-off-by: inocsin <[email protected]>
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
in TensorRT Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
There was a problem hiding this comment.
There are some changes that do not conform to C++ style guidelines:
diff --git a/workspace/core/conversion/converters/impl/topk.cpp b/tmp/changes.txt
index 9ca91a2..dcc83d2 100644
--- a/workspace/core/conversion/converters/impl/topk.cpp
+++ b/tmp/changes.txt
@@ -21,7 +21,8 @@ auto topk_registrations TRTORCH_UNUSED = RegisterNodeConversionPatterns().patter
auto k = args[1].unwrapToInt();
auto dim = args[2].unwrapToInt();
auto largest = args[3].unwrapToBool();
- LOG_DEBUG("Note: sorted argument is not used in TensorRT for aten::topk, results will depend on the value of largest");
+ LOG_DEBUG(
+ "Note: sorted argument is not used in TensorRT for aten::topk, results will depend on the value of largest");
// auto sorted = args[4].unwrapToBool(); # Currently unused
auto selfDim = util::toVec(self->getDimensions());
ERROR: Some files do not conform to style guidelines
Description
Adds support for
aten::topkType of change
Please delete options that are not relevant and/or add your own.
Checklist: