bugGNU Octave - Bugs: bug #55659, s_idx output of ismember uses last...

 
 

bug #55659: s_idx output of ismember uses last occurrence when there are duplicates, Matlab uses first

Submitter:  Travis Arnold <teerav42>
Submitted:  Wed 06 Feb 2019 08:50:15 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  1 - Later Item Group:  Matlab Compatibility
Status:  Patch Submitted Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Wed 05 Jan 2022 10:25:42 PM UTC, comment #4: 

forgot, there's also one current test that produces an error, but matlab produces a warning. Specifically:


%!fail ("ismember ({'1'}, {'1' '2'},'rows')")

gives an error in octave:


>> ismember ({'1'}, {'1' '2'},'rows')

error: ismember: cells not supported with "rows" flag
error: called from
    validsetargs at line 62 column 13
    ismember at line 146 column 10


vs matlab which does:

>> ismember ({'1'}, {'1' '2'},'rows')
Warning: The 'rows' input is not supported for cell array inputs.
> In cellismemberlegacy (line 47)
In cell/ismember (line 65)

ans =

  logical

   1


I don't know if that is a deliberate choice.   for now this patch just adds a comment noting such after that test. if it should issue a warning and provide an output for compatibility, either validsetargs  would have to be adjusted, or ismember would have to work around that case.

(also, v2 patch attached fixing input validation block.)

(file #52618)

Nicholas Jankowski <nrjank>
Group Member
Wed 05 Jan 2022 10:12:24 PM UTC, comment #3: 

attached is a patch that implements the ismember 'legacy' option for returning the first/last index, so as to match curret matlab behavior. Verified BISTs and added ones for 'legacy'. For the most part getting the first result was just a matter of reversing the array order before processing. for 'rows', since lookup isn't used, it just meant calling unique with 'first' instead of 'last' and changing the mapping after the fact. and the complex input handling just needed a switch from checking min to max. other than a few extra if (optlegacy)'s, i think the only overhead is a few flipud's in the !optlegacy paths.

a few issues popped up with processing char arrays, and I added those as 3 xtests at the end, and created bug #61765 (for issorted handling  of arrays, which i think is the main cause of all 3.)

patch updates news.8 with matlab compatibility note as well.

i also noted that this doesn't do anything related to char whitespace handling mentioned in bug #51187, or the different handling of some classes as hinted at in the matlab docs.

(file #52617)

Nicholas Jankowski <nrjank>
Group Member
Mon 29 Jul 2019 09:29:42 PM UTC, comment #2: 

Here is another piece of sample code along with analysis.

For the following code, Matlab returns [0, 0, 2, 1]


A = [5 3 4 2];
B = [2 4 4 4 6 8];
[~,ib] = ismember(A,B)
ib = [0, 0, 2, 1]


Octave, however, returns [0, 0, 4, 1].

The same result can be had in Matlab if the "legacy" option is used with ismember (which uses the algorithm from Matlab versions prior to 2012b).

The difference is that when there are repeated, non-unique values in the set B, Matlab returns the index of the first occurrence while Octave returns the index of the last occurrence.

Debugging further in the ismember.m file, it is the call to lookup() which returns the last index rather than the first index.

Changing lookup() is a lot of work, and likely to have other consequences.

The problem could be worked around in the ismember.m file by sorting the input set in descending order before calling lookup() and then returning


s_idx(tf) = (numel (tf) + 1) - is(s_idx(tf));


But, all of the manipulation seems like a lot of work just to get Matlab compatibility on indices which still work for indexing.

Rik <rik5>
Group administrator
Wed 06 Feb 2019 09:25:04 PM UTC, comment #1: 

It looks like the documentation author may have intended to swap 's' and 'a'


[tf, s_idx] = ismember (s, a)
=> tf = [1, 0]
=> s_idx = [1, 0]


In any case, this was an easy fix which I made here (https://hg.savannah.gnu.org/hgweb/octave/rev/c17bdf3b3841).

Whether the first, or last, index is returned when there are multiple occurrences in the set is a small thing.  The index is only meant to be used with the original set and either convention will work just fine.


s(s_idx_first_duplicate) == s(s_idx_last_duplicate) == a


I'm changing the priority to low, but if you make a changeset for this issue report it can be applied.

Rik <rik5>
Group administrator
Wed 06 Feb 2019 08:50:15 PM UTC, original submission:  

The Octave documentation for ismember gives the following example:


a = {"abc"};
s = {"abc", "def"};
[tf, s_idx] = ismember (a, s)
=> tf = [1, 0]
=> s_idx = [1, 0]


In reality, in Octave this returns


=> tf = 1
=> s_idx = 1


This is consistent with Matlab, so the documentation should be updated to reflect the actual behavior.

However, here is an example where the behavior of ismember is inconsistent with Matlab:


a = {'abc'};
s = {'abc', 'def', 'abc'};
[tf, s_idx] = ismember(a, s)


In Octave this returns


=> tf = 1
=> s_idx = 3


In Matlab it returns


=> tf = 1
=> s_idx = 1


The Matlab documentation for ismember notes that its behavior has changed (though it doesn't say as of which release), noting that "occurrence of indices in s_idx switched from highest to lowest".

This will be a straightforward fix I would imagine, and I might be able to do it myself at some point given that ismember is not a compiled function, but I do not have time for it at the moment and might not for a while.

Travis Arnold <teerav42>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #52618:  ismember_legacy_bug55659_v2.diff added by nrjank (12KiB - application/octet-stream - input validation fix)
file #52617:  ismember_legacy_bug55659.diff added by nrjank (12KiB - application/octet-stream)

 

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

    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.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-01-05 nrjank Attached File- Added ismember_legacy_bug55659_v2.diff, #52618
    2022-01-05 nrjank Attached File- Added ismember_legacy_bug55659.diff, #52617
        StatusConfirmed Patch Submitted
        Operating SystemGNU/Linux Any
    2022-01-03 mmuetzel Dependencies- bugs #61750 is dependent
    2019-07-29 rik5 Dependencies- bugs #56692 is dependent
    2019-07-29 rik5 Priority2 1 - Later
    2019-02-06 rik5 Priority5 - Normal 2
        StatusNone Confirmed
        Release4.4.1 dev
        Summaryismember documentation is misleading, and behavior is incompatible with Matlab s_idx output of ismember uses last occurrence when there are duplicates, Matlab uses first
    2019-02-06 teerav42 Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-b921.
    Corresponding source code