-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tensor minimum/maximum ops #5261
Conversation
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
output = core::Maximum(input, other); | ||
EXPECT_TRUE(output.AllClose( | ||
core::Tensor::Init<float>({{4, 5, 3}, {3, 5, 3}}, device))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least for one case, we must also verify behavior over different dtypes such as int, uint, and bool. You may add this after the Special case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make bool
dtype supported? I think LogicalAnd
and LogicalOr
are equivalent to Minimum
and Maximum
for bool
respectively. @reyanshsolis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 7 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @reyanshsolis, @yuecideng, and @yxlao)
cpp/open3d/core/kernel/BinaryEW.h
line 77 at r2 (raw file):
#endif inline void Add(const Tensor& lhs, const Tensor& rhs, Tensor& dst) {
Clean up previous code, and remove all of these.
cpp/open3d/core/kernel/BinaryEWCUDA.cu
line 61 at r2 (raw file):
const scalar_t* rhs_ptr = static_cast<const scalar_t*>(rhs); if (*lhs_ptr > *rhs_ptr) { *static_cast<scalar_t*>(dst) = *lhs_ptr;
Can we try:
https://docs.nvidia.com/cuda/cuda-math-api/group__CUDA__MATH__INT.html#group__CUDA__MATH__INT
Code quote:
OPEN3D_HOST_DEVICE
cpp/pybind/core/tensor_function.cpp
line 109 at r2 (raw file):
R"(Computes the element-wise maximum of input and other. The tensors must have same data type and device.
nit: remove space
cpp/tests/core/TensorFunction.cpp
line 268 at r2 (raw file):
Previously, yuecideng (Yueci Deng) wrote…
Should we make
bool
dtype supported? I thinkLogicalAnd
andLogicalOr
are equivalent toMinimum
andMaximum
forbool
respectively. @reyanshsolis
Try directly use dispatch template with bool and see if result is correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 8 files reviewed, 3 unresolved discussions (waiting on @reyanshsolis and @yxlao)
cpp/open3d/core/kernel/BinaryEW.h
line 77 at r2 (raw file):
Previously, yxlao (Yixing Lao) wrote…
Clean up previous code, and remove all of these.
Done.
cpp/open3d/core/kernel/BinaryEWCUDA.cu
line 61 at r2 (raw file):
Previously, yxlao (Yixing Lao) wrote…
Can we try:
https://docs.nvidia.com/cuda/cuda-math-api/group__CUDA__MATH__INT.html#group__CUDA__MATH__INT
Done.
cpp/pybind/core/tensor_function.cpp
line 109 at r2 (raw file):
Previously, yxlao (Yixing Lao) wrote…
nit: remove space
Done.
cpp/tests/core/TensorFunction.cpp
line 268 at r2 (raw file):
Previously, yxlao (Yixing Lao) wrote…
Try directly use dispatch template with bool and see if result is correct
Use dispatch template with bool for Max
and Min
conditions. Also add more test cases for these two functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after CI
Reviewed 5 of 7 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @reyanshsolis)
Add two operations
Maximum
,Minimum
in tensor function.This change is