Wed 08 Mar 2017 11:24:42 AM UTC, comment #1:
I implemented the "includenan" flag for min/max and cummin/cummax in two ways: (mainly) in liboctave and (mainly) in libinterp (see the list). Read on for more verbosity.
Concerning the liboctave solution: in the "includenan" case in OP_MINMAX_FCN2, while going through the rows first could make it possible to break the column loop at the first NaN found, I chose to go through the columns first for cache-friendliness and used an OCTAVE_LOCAL_BUFFER to know when to skip rows. A quite similar method was used in OP_ROW_SHORT_CIRCUIT, but the number of buffer updates could reach m * n there (if PRED is always false), while in my case it is at most m. On the other hand, I am not cache-friendly on the buffer itself.
As opposed to my response on the list regarding the libinterp solution:
- It was not necessary to manually go through the array to find the indices of the original NaN values. Instead, I obtained them by calling min/max on the result of isnan, converted to an int8NDArray. Note that NaN values can actually differ for both real (NaN vs. NA) and complex (NaN+2i, NaN+3i, NA+3i) types.
- For two inputs, I did not create all those arrays (x_extended, zeros(size(y)), y_extended, zeros(size(x)) or x_extended_isnan), but called do_bsxfun_op with my own functions which preserve NaNs.
- For two complex input arrays, I did not implement an additional "omitnan" case in libinterp, but modified the underlying octave::math::min/max to omit NaNs for complex types (as with real types) and implemented an "includenan" case in libinterp. I checked that no code using octave::math::min/max relied on their "includenan" comportment for complex types. Apart from the min/max functions, they are only called with complex arguments from _accumarray_min/max_, but even there including NaNs is wrong, not just because of ML incompatibility, but it also produces bugs:
Both patches will also fix the following issues:
the third column can already be processed in the faster loop.
- Complex numbers with equal magnitudes are not ordered by phase angles when calling min/max with a single SparseComplexMatrix or with two arguments, either full or sparse.
- The min/max functions between a *Matrix and a scalar were implemented to call the scalar-matrix versions, although the operations are not commutative: between two NaN values, the first one should be returned (regardless of the NaN flag).
- In the min/max functions between two [Float]ComplexMatrices, it is checked for each column if all of the elements are real, in which case only the real parts are compared. Therefore, negative numbers on real columns will not be compared by magnitude.
- mx_inline_xmin/xmax between a real NaN scalar and a vector keep the NaN values in the vector even though the scalar is the first. This happens because of the second F<T> in DEFMINMAXSPEC.
- cummin/cummax don't return sparse matrices when called with sparse inputs. The patches include Sparse[Complex]Matrix::cummin/cummax methods whose complexity only depends on the number of positions with nonzero values (in the input or output matrix) if the index is not requested.
- subsasgn cannot be used on a float complex scalar if the rhs is a double scalar, a complex scalar or an empty matrix. I was incidentally trying to do this for a corner case. For example:
- In max.cc, the dimension test for max mistakenly calls min instead. Also, this test:
should expect: [1 2 2i 2i].
By using the attached random test generators, I tested the two solutions both one vs. another and for sparse matrices vs. full 2D arrays.
Please let me know if you have any questions.
|