bugGNU Octave - Bugs: bug #65527, sort should produce an error for...

 
 

bug #65527: sort should produce an error for nonscalar dim

Submitter:  None
Submitted:  Thu 28 Mar 2024 07:05:51 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Missed Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 8.4.0
Operating System:  * Any Fixed Release:  10.1.0
Planned Release:  10.1.0
* Mandatory Fields

Post a Comment

Add a New Comment Rich Markup
   

Discussion

Jump to the original submission

Mon 01 Apr 2024 03:22:47 PM UTC, comment #17: 


comment #16:

> obviously would take a bit more effort to code those.  that would be a feature change that should get pushed to octave 10.  improving input validation if it doesn't change critical features could go to stable for 9.x. Based on the change to sort once it's settled how best to do it that the 9.x change would be fairly simple to apply to all of the functions.


In general, 'omitnan' probably needs to be addressed in the base Array class in liboctave.  Once it is implemented there, then all user-facing functions in libinterp will automatically inherit the additional feature.  But, that is a reasonably large coding project and would need to take place on the development branch.

I would say that improving input validation is a "nice to have" rather than a "need to have".  Even though changes appear small, there may be users relying on current silent acceptance of certain inputs, and thus this change should also go on development branch.

The particular problem reported about sort has been solved.  Marking bug as Fixed and closing report.

Rik <rik5>
Group administrator
Mon 01 Apr 2024 12:42:53 PM UTC, comment #16: 

obviously would take a bit more effort to code those.  that would be a feature change that should get pushed to octave 10.  improving input validation if it doesn't change critical features could go to stable for 9.x. Based on the change to sort once it's settled how best to do it that the 9.x change would be fairly simple to apply to all of the functions.

Nicholas Jankowski <nrjank>
Group Member
Mon 01 Apr 2024 06:31:56 AM UTC, comment #15: 

comment #11:

>
> (where matlab takes a vecdim but we don't yet)
> sum, min, max ,
>

Is it too much involved to add 'vecdim' and 'omitnan' options to these three functions so that they become consistent with how mean, median, etc behave?

Andreas Bertsatos <pr0m1th3as>
Sun 31 Mar 2024 11:31:16 PM UTC, comment #14: 

retitle this report or open a new one for the others?

Nicholas Jankowski <nrjank>
Group Member
Sat 30 Mar 2024 03:13:33 AM UTC, comment #13: 

I think we should fix all of these code patterns based on the answer to the Discourse thread about whether we should be using int or octave_idx_type.

Rik <rik5>
Group administrator
Sat 30 Mar 2024 01:52:22 AM UTC, comment #12: 

oops, submit to soon.  there are a bunch more in max.cc and data.cc:

max.cc: cummin, cummax

data.cc: any, all, cumsum, cumprod, prod, sum, sumsq, vec, nth_element, diff,

Nicholas Jankowski <nrjank>
Group Member
Sat 30 Mar 2024 01:43:43 AM UTC, comment #11: 

thanks for the quick fix.  In looking for how other compiled functions handled this, I noticed the same vector dim input validation issue appears in other functions that aren't intended to take a vecdim. going a search for all files with a @var{dim} and testing them with a vector dim comes up with: 

(where matlab takes a vecdim but we don't yet)
sum, min, max ,

(which are supposed to be scalar only).
fft, ifft, filter, cellslices, dot,  cross (m file, but actually changes output, needs another look)

Nicholas Jankowski <nrjank>
Group Member
Fri 29 Mar 2024 09:47:26 PM UTC, comment #10: 

This was easy, and it matches Matlab which emits an error if the DIM input is not a scalar.  See changeset https://hg.savann ... /rev/59ee9d370307.

Marking as Ready for Test.

Rik <rik5>
Group administrator
Fri 29 Mar 2024 08:15:30 PM UTC, comment #9: 

I tried to guard against non-integer values of DIM but discovered a new bug with xint_value(), which is used elsewhere in the same file and affects other functions like all(), so "all (true (3), pi)" does not complain. I will file a separate bug report for that.


Arun Giridhar <arungiridhar>
Group Member
Fri 29 Mar 2024 07:54:40 PM UTC, comment #8: 

Better draft patch attached.

This is on default so far, not stable.



(file #55901)

Arun Giridhar <arungiridhar>
Group Member
Fri 29 Mar 2024 07:31:54 PM UTC, comment #7: 

Draft patch attached with some new BISTs.



(file #55900)

Arun Giridhar <arungiridhar>
Group Member
Fri 29 Mar 2024 12:42:25 PM UTC, comment #6: 

sortrows is a separate function with a separate purpose where a vecdim makes sense.

rather than catch with an error, you could define an octave extension over what matlab does that creates a meaning for a vector-dimension, perhaps as suggested by @Anon in comment #5, but you'd need to ensure not breaking compatibility with any intended functionality including all sort options.

I don't see any particular optimization that would occur by specifying sort that way, when it would just be a recursive call to sort and a programmatically clear means of achieving the same is what's written in the last comment. That can be used with the regular dim argument to make algorithmically flexible sort dimensions. 

For now I'm going to keep this report focused on a need to catch and produce a sensible error for incorrect input to sort.

Nicholas Jankowski <nrjank>
Group Member
Fri 29 Mar 2024 12:25:28 PM UTC, comment #5: 

Sortrows only sorts the rows, not other dimensions.

To sort A along direction X, then Y to tiebreak X, then Z to tiebreak X and Y:

A = sort (A, Z);
A = sort (A, Y);
A = sort (A, X);

Or combine all:

A = sort (sort (sort (A, Z), Y), X);

Anonymous
Fri 29 Mar 2024 07:32:24 AM UTC, comment #4: 

"sortrows" in both Octave and MATLAB can be used with a vector that indicated the order of the columns.

Anonymous
Fri 29 Mar 2024 01:07:17 AM UTC, comment #3: 

i see no documentation in octave or matlab that would lead me to expect that such a syntax would work.

Nicholas Jankowski <nrjank>
Group Member
Thu 28 Mar 2024 11:04:44 PM UTC, comment #2: 

Sort order vector [X Y Z] means sort first in X then in Y (to tie break X) then in Z (to tie break X and Y). Not possible with sort?

Anonymous
Thu 28 Mar 2024 09:03:11 PM UTC, comment #1: 

i don't think it is expected for sort to take a vector input for dim. while there are some functions that do this, i'm not even sure how you'd interpret such an ordering instruction.  From a compatibility perspective, matlab docs specify it is a scalar and inputting a vector produces an error.

I think this is just a case of an uncaught erroneous input.  the function is just performing a sort using dim(1). 

this isn't a case of documentation, but the function should be producing an error. Changing category to "Missed Error or Warning" and updating title.

Nicholas Jankowski <nrjank>
Group Member
Thu 28 Mar 2024 07:05:51 PM UTC, original submission:  

The documentation for Octave's sort should describe the result when the sort order is a vector.

A = sort (A, [2 3 1]);


Anonymous

 

Attached Files

Attached Files
file #55901:  sort.patch added by arungiridhar (5.1KiB - text/x-patch)

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

Attach Files:
   
   
Comment:
   

 

Dependencies

This item does not depend on any other items.

No items depend on this one.

 

Mail Notification Carbon-Copy List

Carbon-Copy List
  • -email is unavailable- added by pr0m1th3as (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by arungiridhar (Updated the item)
  • -email is unavailable- added by nrjank (Posted a comment)
  •  

    Votes

    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.

     

    History

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-04-01 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 10.1.0
    2024-03-29 rik5 StatusConfirmed Ready For Test
        Planned ReleaseNone 10.1.0
    2024-03-29 arungiridhar Attached File- Added sort.patch, #55901
    2024-03-29 arungiridhar Attached File#55900 Removed
    2024-03-29 arungiridhar Attached File- Added sort.patch, #55900
    2024-03-28 nrjank CategoryNone Octave Function
        Item GroupNone Missed Error or Warning
        StatusNone Confirmed
        Operating SystemMicrosoft Windows Any
        SummaryMissing documentation for sort sort should produce an error for nonscalar dim

    Back to the top

    Powered by Savane 3.16.
    Corresponding source code