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

 
 

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

Submitted by:  David Spies <dspyz>
Submitted on:  Mon 10 Mar 2014 12:58:07 AM UTC  
 
Category: NonePriority: 5 - Normal
Status: NonePrivacy: Public
Assigned to: NoneOpen/Closed: Open

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

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

 

(Jump to the original submission 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>
Project 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>
Project 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>
Project 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 File(s):
   
   
Comment:
   

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

     

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

     

     

    Follow 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 21 Mar 2014 07:34:47 PM UTCdspyzAttached File-=>Added find_cleanup.diff, #30994
    Thu 20 Mar 2014 12:29:03 AM UTCdspyzAttached File-=>Added find_cleanup.diff, #30952
    Mon 10 Mar 2014 09:04:53 PM UTCdspyzAttached File-=>Added find4.diff, #30849
    Mon 10 Mar 2014 12:58:07 AM UTCdspyzAttached File-=>Added find4.diff, #30842

    Back to the top


    Powered by Savane 3.1-cleanup