Update

[ghstack-poisoned]

Author: swolchok

kernels/portable/cpu/op_argmax.cpp

@@ -50,10 +50,7 @@ Tensor& argmax_out(
     for (const auto out_ix : c10::irange(out.numel())) {
       std::tuple<CTYPE, long> acc = reduce_over_dim<CTYPE>(
           [](CTYPE v, long ix, CTYPE acc_val, long acc_ix) {
-            // the below condition as written is equivalent to
-            // !isnan(accval) && (isnan(v) || v > acc_val). See
-            // argument in op_argmin.cpp.
-            if (!std::isnan(acc_val) && !(v <= acc_val)) {
+            if (!std::isnan(acc_val) && (std::isnan(v) || v > acc_val)) {
               acc_val = v;
               acc_ix = ix;
             }

kernels/portable/cpu/op_argmin.cpp

@@ -50,17 +50,7 @@ Tensor& argmin_out(
     for (const auto out_ix : c10::irange(out.numel())) {
       std::tuple<CTYPE, long> acc = reduce_over_dim<CTYPE>(
           [](CTYPE v, long ix, CTYPE acc_val, long acc_ix) {
-            // the below condition as written is equivalent to !isnan(accval) &&
-            // (isnan(v) || v < acc_val). cases:
-            // - if neither acc_val nor v is NaN, !(v >= acc_val) is
-            //   trivially equivalent to v < acc_val.
-            // - if acc_val is NaN, the whole thing is trivially false.
-            // - if acc_val is not NaN and v is NaN, then v >= acc_val
-            // - is false because all comparisons involving NaN are
-            // - false, so the result is true. The result is trivially
-            // - true for the above condition that uses isnan(v) as
-            // - well.
-            if (!std::isnan(acc_val) && !(v >= acc_val)) {
+            if (!std::isnan(acc_val) && (std::isnan(v) || v < acc_val)) {
               acc_val = v;
               acc_ix = ix;
             }

kernels/test/op_argmax_test.cpp

@@ -90,16 +90,3 @@ TEST_F(OpArgmaxTest, SanityCheckNullDim) {
   EXPECT_TENSOR_EQ(out, expected);
   // clang-format on
 }
-
-TEST_F(OpArgmaxTest, FirstNaNWins) {
-  TensorFactory<ScalarType::Float> tf_float;
-  Tensor in = tf_float.make({4}, {1, NAN, -4, NAN});
-
-  TensorFactory<ScalarType::Long> tf_long;
-  Tensor out = tf_long.zeros({});
-  Tensor expected = tf_long.make({}, {1});
-
-  Tensor ret = op_argmax_out(in, {}, false, out);
-  EXPECT_TENSOR_EQ(out, ret);
-  EXPECT_TENSOR_EQ(out, expected);
-}

kernels/test/op_argmin_test.cpp

@@ -90,16 +90,3 @@ TEST_F(OpArgminTest, SanityCheckNullDim) {
   EXPECT_TENSOR_EQ(out, expected);
   // clang-format on
 }
-
-TEST_F(OpArgminTest, FirstNaNWins) {
-  TensorFactory<ScalarType::Float> tf_float;
-  Tensor in = tf_float.make({4}, {1, NAN, -4, NAN});
-
-  TensorFactory<ScalarType::Long> tf_long;
-  Tensor out = tf_long.zeros({});
-  Tensor expected = tf_long.make({}, {1});
-
-  Tensor ret = op_argmin_out(in, {}, false, out);
-  EXPECT_TENSOR_EQ(out, ret);
-  EXPECT_TENSOR_EQ(out, expected);
-}