[Octave-bug-tracker] [bug #63962] perms - Performance - Usage of native C++ algorithm helpful

[Hendrik K]

Follow-up Comment #18, bug #63962 (project octave):

@Arun-

Thanks. Looks good and I am fine with the changes including the warning and
then giving back an empty octave value. 

For the commit log you can use my full name "Hendrik Koerner
<koerhen@web.de>"

i)
I did tow last content changes: Changing the AND order of the check for
"unique" for cells allowing for a faster execution time as a simple int
comparison is faster than an octave class "is_equal" method call comparison -
note that it will not matter much as it is already very fast in C++. 
((Ar[i].is_equal (Ar[j]) && (myvidx[j] > myvidx[i])) -->

((myvidx[j] > myvidx[i]) && (Ar[i].is_equal (Ar[j]))

Also checking that the number of input arguments is not more than 2 (and not
just less than one). I had this in the original code but lost it during the
last iteration.


ii)
@ "Setting do_sort to false for the case of booleans?"
This is by design following the existing functionality and assert testings:
for historical reasons logicals are not sorted but value but by order of input
(in contrast to numericals) which makes some sense when you think about it.
E.g. 
%!assert (perms (logical ([1 0])), logical ([0 1;, 1 0]))
%!assert (perms (logical ([0 1])), logical ([1 0; 0 1]))

So for historical compatibility I kept this but I now give some more comments
    // Do not sort logicals by value as the other numerical data types.
    // This reflects past design design decisions using the order positioning
    // for logicals which makes more sense than ordering logicals by value.


iii)
Note that there is a reopening and re-discussion of the perms ordering:
https://savannah.gnu.org/bugs/?50426
I guess that eventually octave may support the Matlab convention to have
reverse ordering by vector position for everything and not just for logicals
(and never by value like now)
The change in perms.cc will be minimal: 
a) Change the default value of bool do_sort = true to false (keep the
functionality even though not used) in GetPerms
b) Remove providing the do_sort flag and the comment in the (clname ==
"logical") 
b) Remove or change the assert (perms (1:5), perms ([2 5 4 1 3]'))

One could already change this now / wait until a decision is formed and solve
50426 together with 63962 referencing each other in the bugs. 
Or solve it separately: so migrate now and make later a second migration for
50426 once agreed.
Your call and not much difference although in general a 1:1 relation between
bug and migration from the latest versioned state to the solved state may be
easier to trace.


PS: The switch from British to American English code comments is fine as well:
In British English, to cater FOR something means to take it into account. In
American English, you say you cater TO something. :-)
  

(file #54538)

    _______________________________________________________

Additional Item Attachment:

File name: patch_63962_v4                 Size:21 KB
    <https://file.savannah.gnu.org/file/patch_63962_v4?file_id=54538>



    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?63962>

_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/