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

 
 

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

Submitted by:  CH <atcl>
Submitted on:  Sun 08 Jan 2017 08:19:30 PM UTC  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Feature Request
Status: Patch SubmittedAssigned to: None
Originator Name: Open/Closed: Open
Release: devOperating System: Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

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 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:
  • 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:
  • 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:

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.

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 File(s):
   
   
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
  • -unavailable- added by avinoam (Updated the item)
  • -unavailable- added by stefanmirea (Updated the item)
  • -unavailable- added by atcl (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 22 Nov 2017 05:13:08 AM UTCavinoamStatusNone=>Patch Submitted
    Fri 17 Mar 2017 10:36:47 PM UTCrik5Dependencies-=>bugs #50571 is dependent
    Wed 08 Mar 2017 11:23:49 AM UTCstefanmireaAttached 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.1-cleanup1