I tried with "NDArray A" and "const NDArray A" and the difference in execution time is about the same but the percentage change is less significant when using the non-const NDArray because the overhead of the indexing operation is much higher.

It seems to me that we could do slightly better with a single loop that does both operations but I don't know that it is worth the trouble.

Also, I guess we should be careful about the difference between "if (val)" and "if (all (val))", not just when val might be empty but also if it might contain NaN elements.  I wouldn't have thought about the latter case before today.

Beware the cost of checking the reference count with the "A(i)" indexing.  You might want to avoid that by using "const NDArray A = ..." or by using "args(1).array_value().data()".

Also, how smart is GCC these days?  Does it compile the two-loop case to be essentially the same as the single loop?  What happens if each loop in the two loop case is defined in a separate function as I think we have in Octave?

I don't think looping over the array twice is that big a deal because it is what is inside the body of the loop which is taking the time rather than the loop structure itself (at least in C++, not Octave).

I made a test case as a .oct file.  This is attached as loop2.cc and shown below.

#include <octave/oct.h>

DEFUN_DLD (loop2, args, nargout,
           "Test single or double loop equivalent to all()\n"
           "ARG1 : TRUE/FALSE, do 1 combined loop?\n"
           "ARG2 : numeric array")
{
  if (args.length () != 2)
    print_usage ();

  bool do_1_loop = args(0).bool_value ();
  NDArray A = args(1).array_value ();
  octave_idx_type N = A.numel ();
  bool retval = true;

  if (do_1_loop)
    {
      for (octave_idx_type i = 0; i < N; i++)
        {
          if (std::isnan (A(i)))
            error ("loop2: Can not convert NaN to logical value");
          if (! A(i))
            retval = false;
        }
    }
  else
    {
      for (octave_idx_type i = 0; i < N; i++)
        {
          if (std::isnan (A(i)))
            error ("loop2: Can not convert NaN to logical value");
        }
      for (octave_idx_type i = 0; i < N; i++)
        {
          if (! A(i))
            retval = false;
        }
    }

  return ovl (retval);
}

I then ran the benchmark and it is basically equivalent.

octave:7> tic; for i = 1:100, loop2 (0,x); end; toc
Elapsed time is 0.385158 seconds.

octave:10> tic; for i = 1:100, loop2 (1,x); end; toc
Elapsed time is 0.382482 seconds.

(file #54039)

But if you do 'isnan (X) && all (X)' for a large array, you are looping over the array twice.  I'm proposing that we combine those operations in a single function that only loops once.

The function tree_evaluator::visit_if_command_list in pt-eval.cc calls tree_evaluator::is_logically_true, which calls octave_value::is_true.  If you've already called "all(data)" and data is a vector, then it is fast because you are looking at a scalar value.  For a double array, you end up in octave_base_matrix<MT>::is_true in ov-base-mat.cc:

// Return true if this matrix has all true elements (non-zero, not NA/NaN).
template <typename MT>
bool
octave_base_matrix<MT>::is_true (void) const
{
  bool retval = false;
  dim_vector dv = m_matrix.dims ();
  int nel = dv.numel ();

  if (nel > 0)
    {
      MT t1 (m_matrix.reshape (dim_vector (nel, 1)));

      if (t1.any_element_is_nan ())
        octave::err_nan_to_logical_conversion ();

      if (nel > 1)
        warn_array_as_logical (dv);

      boolNDArray t2 = t1.all ();

      retval = t2(0);
    }

  return retval;
}

Reshape should be fast because it should just modify the dimensions, not copy any data.  The slow part is that we are scanning for NaNs and then again for zero values.  For speed, those could be combined in a single operation.  It should probably be defined in a separate function the Array class rather than doing the operation in octave_base_matrix<MT>::is_true.

This comparison also points out that "if (all (NaN))" is not the same as "if (NaN)".  Is that difference compatible with Matlab?

We should rename this project from Octave to "pitfall".