bugGNU Octave - Bugs: bug #50007, NaN aware min and max via...

 
 

bug #50007: NaN aware min and max via "includenan"

Submitter:  CH <atcl>
Submitted:  Sun 08 Jan 2017 08:19:30 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Patch Submitted Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Tue 23 Nov 2021 09:25:48 PM UTC, comment #2: 

it's been a while since this was written, and the functions needing the includenan flag for compatibility have probably been edited since then, so the patch probably needs to be refreshed in addition to determining if it's fit for inclusion.

The original list referred to in comment #1 can now be found here from the maintainers mailing list:
https://lists.gnu.org/archive/html/octave-maintainers/2017-01/msg00147.html


note a list of potential nanflag functions was also given in bug #50571.  if this patch does the job, perhaps other functions can be similarly modified.

Nicholas Jankowski <nrjank>
Group Member
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:


>> % calling with FUNC = @max and VALS including a NaN makes all the values in the result NaN
>> accumarray ([1; 1; 2; 2], [1; 2i; 3; NaN], [], @max)
ans =

   NaN
   NaN

>> % calling with FUNC = @min and just complex VALS makes all the values in the result NaN
>> accumarray ([1; 1; 2; 2], [i; 2i; 3; 4], [], @min)
ans =

   NaN
   NaN


Both patches will also fix the following issues:

  • The min/max default behaviour is not "omitnan" when called with a Sparse[Complex]Matrix on the second dimension and one of the rows has all the elements nonzero and the first one NaN:


min (sparse ([1 1], [1 2], [NaN 1]), [], 2) % expected sparse (1), got sparse (NaN)


  • When calling min/max for a SparseComplexMatrix on the first dimension and one of the columns has all the elements NaN, the result is 0 for that column:


min (sparse ([1 2], [1 1], [NaN+i NaN])) % expected sparse (NaN+i), got sparse (0)


  • OP_MINMAX_FCN2 and OP_CUMMINMAX_FCN2 in mx-inlines.cc were supposed to jump to the faster code (according to the comment above them) as soon as r contained no NaN value, but the actual code waits to find in v a column without NaNs. For the following matrix:


max ([1 NaN 3; NaN 2 NaN], [], 2)

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:


s = single (3);    s(1) = 4  % float scalar = double scalar => ok
s = 2+3i;          s(1) = 4  % complex scalar = double scalar => ok
s = single (2+3i); s(1) = 4  % float complex scalar = double scalar => error

s(false) = zeros(0, 1) % error if s is a float complex scalar; works for other scalar types


  • In max.cc, the dimension test for max mistakenly calls min instead. Also, this test:


%! x = [1, 2, 3, 4];  y = fliplr (x);
%! assert (min (x, 2i), [2i 2i 3 4]);

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.

Stefan Mirea <stefanmirea>
Sun 08 Jan 2017 08:19:30 PM UTC, original submission:  

MATLAB's min and max functions allow passing an "includenan" flag as a third argument which cause NaN's in the first two arguments to be propagated.

CH <atcl>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #39936:  liboctave.patch added by stefanmirea (249KiB - text/x-patch)
file #39937:  tester.zip added by stefanmirea (5KiB - application/zip)
file #39938:  libinterp.patch added by stefanmirea (96KiB - text/x-patch)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by avinoam (Updated the item)
  • -email is unavailable- added by stefanmirea (Updated the item)
  • -email is unavailable- added by atcl (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-11-22 avinoam StatusNone Patch Submitted
    2017-03-17 rik5 Dependencies- bugs #50571 is dependent
    2017-03-08 stefanmirea Attached File- Added liboctave.patch, #39936
        Attached File- Added tester.zip, #39937
        Attached File- Added libinterp.patch, #39938

    Back to the top

    Powered by Savane 3.14-3b9d.
    Corresponding source code