patchGNU Octave - Patches: patch #8386, Fixed up "find" function...

 
 

patch #8386: Fixed up "find" function with templates and fourth parameter

Submitter:  David Spies <dspyz>
Submitted:  Mon 10 Mar 2014 12:58:07 AM UTC
   
 
Category:  Core : other Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 21 Mar 2014 07:34:47 PM UTC, comment #7: 

Oh of course, you're right.  I needed std::min.  Ok, here's the new diff.



(file #30994)

David Spies <dspyz>
Thu 20 Mar 2014 05:08:26 AM UTC, comment #6: 

I think you need to say std::min and ensure that the two arguments have the same type.

If I could eliminate the use of CamelCase for the type names, I would.  I would prefer to avoid adding any more.

John W. Eaton <jwe>
Group administrator
Thu 20 Mar 2014 05:06:12 AM UTC, comment #5: 

Functions like max, min, sum, any, etc., all work along dimension 1 by default and return an array that has one fewer dimension than the original array.  So it makes sense for those functions to accept an argument that specifies the dimension over which to operate.  Although the DIM argument of these functions affects the shape of the result, it does not specify it directly.  The shape of the original array is also a factor (consider the case of arrays of more than two dimensions).

The find function just returns a collection of linear indices based on column major ordering.  The shape of that collection is not related to the shape of the original array.  So it makes sense for the result to always just have a single orientation.

I don't understand what you mean by "the only stable use of "find" on a matrix is to just always pair it with a reshape".

John W. Eaton <jwe>
Group administrator
Thu 20 Mar 2014 04:08:03 AM UTC, comment #4: 

Sorry, I never finished reading your comments (or if I did I forgot).  I was too concerned with the first part to get through the rest.

If I don't include "min" myself, then I get:

corefcn/find.cc: In function 'octave_idx_type min_with_nnz(const PermMatrix&, octave_idx_type)':
corefcn/find.cc:220:39: error: call of overloaded 'min(octave_idx_type&, octave_idx_type&)' is ambiguous
corefcn/find.cc:220:39: note: candidates are:
../liboctave/array/chNDArray.h:108:31: note: charNDArray min(char, const charNDArray&)
../liboctave/array/chNDArray.h:109:31: note: charNDArray min(const charNDArray&, char)
../liboctave/array/chNDArray.h:110:31: note: charNDArray min(const charNDArray&, const charNDArray&)

If I explicitly specify the type (ie min<octave_idx_type>(a,b)), I get:
corefcn/find.cc: In function 'octave_idx_type min_with_nnz(const Sparse<T>&, octave_idx_type)':
corefcn/find.cc:211:31: error: expected primary-expression before '>' token

#include <algorithm> doesn't seem to fix it.

I can't find anywhere I used CamelCase (except ItType, but I don't think you're referring to that.  It's a type parameter.  It acts like a type like PermMatrix and FloatNDArray etc. which all use CamelCase as far as I can tell)

David Spies <dspyz>
Thu 20 Mar 2014 12:29:03 AM UTC, comment #3: 

Yes, here's the diff for just the cleanup/bugfixes to find without the extra fourth parameter.  The reason I thought the fourth parameter is necessary is essentially summed up in https://savannah.gnu.org/bugs/?func=detailitem&item_id=40296

Specifically, there are quite a few functions (max,min,sum,any,all,product,find,and logical indexing ...) whose default return value is "almost" always a column vector with the edge-case being when the input happens to be a row-vector (and strange arbitrary behavior when things with a zero-dimension are passed).  When writing a "real" project (ie something that's more than a couple lines of scripting), this is extremely frustrating behavior because I almost always forget to reshape the output to deal with those edge-cases.

All of these functions except find and logical indexing can take an extra "dimension" parameter that clarifies whether you want to take the max/sum/etc. over rows or columns and also fixes the dimension of the return value.  It seems only reasonable that find and logical indexing should also provide that capability.  Otherwise, the only stable use of "find" on a matrix is to just always pair it with a reshape.


(file #30952)

David Spies <dspyz>
Wed 12 Mar 2014 06:54:21 PM UTC, comment #2: 

Since the extra outputs were undocumented and I couldn't find any uses in Octave or Octave Forge, I suppose it would be OK to simply remove them instead of deprecating, warning, then deleting a few releases from now.

I'm not convinced that the additional fourth input argument is needed.  Reshaping is cheap, and Matlab doesn't have this option, does it?  Other functions don't have this option, so why should find?

Could you please separate the addition of the fourth argument from the other patch and submit two separate patches?

It would also be helpful if you could follow the prevailing coding style (primarily the GNU coding style, plus there are a few other tips here: http://www.gnu.org/software/octave/doc/interpreter/C_002b_002b-Sources.html#C_002b_002b-Sources).  We also try to avoid CamelCase.

I think the reason that you didn't find a "min" function in the Octave sources is because there is one in the C++ standard library, defined in <algorithm>.  Is there some reason that won't work in this context?

John W. Eaton <jwe>
Group administrator
Mon 10 Mar 2014 09:04:53 PM UTC, comment #1: 

Sorry, there were still some bugs.  I fixed them.  It now passes all the same unit tests as before and works the same as Matlab for multi-dimensional arrays.

Additionally I noticed another bug in the current "find".  If given nargout >= 6 with a sparse array, Octave crashes.  I removed support for 5 return values (with the last two being nrows,ncols), because it's:

a) unnecessary
b) undocumented
c) unsupported for full matrices
d) unimplemented in Matlab



(file #30849)

David Spies <dspyz>
Mon 10 Mar 2014 12:58:07 AM UTC, original submission:  

I cleaned up the "find" function adding templates to channel all calls to 'find' for all types into the same template function.

As a side-effect, this fixes the following matlab/full-matrix incompatibility:
octave:3> a = sparse(0,0);
octave:4> size(find(a))
ans =

  1. 1


(Matlab and full matrices give 0 0)

Additionally I added a fourth parameter to specify the desired dimension of the output vectors:
octave:3> a = eye(3);
octave:4> [i,j,v] = find(a,Inf,"first",2)
i =

1 2 3

j =

1 2 3

v =

1 1 1

Could someone please look over my changes?

Thanks

David Spies <dspyz>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #30994:  find_cleanup.diff added by dspyz (17KiB - text/x-patch)
file #30952:  find_cleanup.diff added by dspyz (17KiB - text/x-patch)
file #30849:  find4.diff added by dspyz (23KiB - text/x-patch)
file #30842:  find4.diff added by dspyz (47KiB - text/x-patch)

 

Depends on the following items: None found

Digest:
   patch dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by siko1056 (Updated the item)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by dspyz (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 logged-in users can vote.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-01-18 siko1056 CategoryNone Core : other
    2020-01-18 siko1056 Dependencies- patch #8417 is dependent
    2014-03-21 dspyz Attached File- Added find_cleanup.diff, #30994
    2014-03-20 dspyz Attached File- Added find_cleanup.diff, #30952
    2014-03-10 dspyz Attached File- Added find4.diff, #30849
    2014-03-10 dspyz Attached File- Added find4.diff, #30842

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code