patchGNU Octave - Patches: patch #10440, Allow unique to process cellstr...

 
 

patch #10440: Allow unique to process cellstr arrays with the 'rows' option

Submitter:  Andreas Bertsatos <pr0m1th3as>
Submitted:  Sun 17 Mar 2024 01:48:11 AM UTC
   
 
Category:  Core : new feature Priority:  5 - Normal
Status:  Wont Do Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 21 Feb 2025 03:34:45 PM UTC, comment #16: 

Attaching here for future reference:
1) the patched version of `unique` with the extra functionality of 'rows' option applying on cellstr arrays as well.
2) the diff patch relative to the current version (as of today)


(file #56936, file #56937)

Andreas Bertsatos <pr0m1th3as>
Fri 21 Feb 2025 02:41:34 PM UTC, comment #15: 

Please also post the diffs relative to the version you started with so that if we revisit this issue later, we don't have to try to figure out what the changes were.

John W. Eaton <jwe>
Group administrator
Fri 21 Feb 2025 02:20:54 PM UTC, comment #14: 

thanks for the follow up.  feel free to attach a version of unique.m with that feature to this report.  I'll close for now and we can revisit if things change.

Nicholas Jankowski <nrjank>
Group Member
Thu 20 Feb 2025 11:15:45 PM UTC, comment #13: 

I'd rather skip this patch entirely than creating a new function for the extended functionality or introduce another named option, which will complicate its usage.

I am already using the patched version as a private function in the statistics package for a future release than I've been working for quite some time now. So it's not an issue if we just skip this patch entirely for core Octave. I just thought that it might be useful for other users if we add this functionality into core Octave. Perhaps, MATLAB might add this feature in the future so we can come back to this in the future if required.

Thanks everyone for the attention and help with this patch.

Andreas Bertsatos <pr0m1th3as>
Thu 20 Feb 2025 08:13:32 PM UTC, comment #12: 

discussed at dev mtg today,  there's reluctance to add this as a capability that changes expected behavior in matlab. can it be added as a non-conflicting extension in some way?  either by creating a unique_cellstr function, or using a differently named option? then identical behavior could be maintained for anything that produces output in matlab (since unique(foo,'rows') does produce an output if foo is a cellstr).

Nicholas Jankowski <nrjank>
Group Member
Thu 13 Feb 2025 07:40:46 PM UTC, comment #11: 

Here is an updated patch that applies cleanly (or I think so) to the latest sources after the changes in the NEWS.10.md from my other patch related pkg function.

I hope this eases the hassle to include this changeset for 10.1

(file #56885)

Andreas Bertsatos <pr0m1th3as>
Mon 10 Feb 2025 04:31:46 PM UTC, comment #10: 


comment #9:

> ... If folks coming from ML rely on "rows" doing nothing in case of cell arrays, well, they'll need to adapt.


My initial motivation for this patch was some work on text related ML I did some time ago (the same work that made me write the c++ editDistanceto function in the statistics package). So I think that people doing ML and require some sort of string tokenization most likely will find this functionality quite handy.

Andreas Bertsatos <pr0m1th3as>
Mon 10 Feb 2025 03:05:55 PM UTC, comment #9: 


> ... i've been waiting on someone else to weigh in on whether that's fine.

In that case I'll weigh in :-)

I think the patch is fine. If folks coming from ML rely on "rows" doing nothing in case of cell arrays, well, they'll need to adapt.
As to myself, I'm much in favor of this change, it could have helped me much in several situations, and it could in future situations as well.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 10 Feb 2025 02:41:38 PM UTC, comment #8: 

matlab explicitly states in its help that


 = unique(A,___,'rows') and C = unique(A,'rows',___) treat each row of A as a single entity and return the unique rows of A in sorted order. You must specify A and optionally can specify setOrder or occurrence.

The 'rows' option does not support cell arrays.


which means that when a cell array is passed it is ignored, and for cell arrays unique(C, 'rows') is equivalent to unique(C).  in octave this change would make unique (C,'rows') produce different results than unique(C). 

it's a minor thing, and we could and a compatibility note highlighting this deliberate difference in the function help. but  with that one sentence in matlab's help i've been waiting on someone else to weigh in on whether that's fine.

Nicholas Jankowski <nrjank>
Group Member
Mon 10 Feb 2025 12:17:23 PM UTC, comment #7: 

Extended functionality is (should) not regarded as incompatibility.

Andreas Bertsatos <pr0m1th3as>
Tue 04 Feb 2025 04:47:02 PM UTC, comment #6: 

i think we were still waiting to get some feedback on whether this change to base behavior was acceptable as it could be seen as introducing an incompatibility.

Nicholas Jankowski <nrjank>
Group Member
Sat 01 Feb 2025 10:15:24 AM UTC, comment #5: 

Can we go ahead and apply this patch before Octave 10 release?

Andreas Bertsatos <pr0m1th3as>
Mon 14 Oct 2024 04:11:20 PM UTC, comment #4: 

updated patch applies cleanly to current default tip and adds a NEWS message.  BIST ordering/locations was only significant change from last patch.

I can't imagine a good case for anyone else relying on the prior

unique({cellstring}, "rows") ==> unique({cellstring})

behavior. But this behavior along with a warning is the current Octave and Matlab behavior.  Will still await any other input on whether to add this.

(file #56529)

Nicholas Jankowski <nrjank>
Group Member
Mon 14 Oct 2024 03:33:00 PM UTC, comment #3: 

I think we were waiting on concurrence regarding adding the new option and if there is any potential compatibility concern.

there were other recent changes to unique.m, so the patch probably has to be recast against the current state. 

I can give that a quick peek.  changing to Need Info.

Nicholas Jankowski <nrjank>
Group Member
Sun 13 Oct 2024 07:12:21 PM UTC, comment #2: 

This has been "In progress" for over half a year. What needs to be done to get this patch report finished?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 17 Mar 2024 11:43:46 PM UTC, comment #1: 

here's a modified patch which will apply cleanly to the current default branch. 

this seems like a worthwhile extension to unique.  I wasn't able to break it or cause any other problems after the change.

There will be a potential compatibility issue, as currently for a cellstring array A, unique(A,'rows') returns the same as unique(A). Matlab is also pretty explicit in it's help for unique that 'rows' does not work for cell arrays. Does anyone know if there are particular problems supporting cellstrings would cause?

running the test suite now to see if it triggers any other issues, but don't expect any.

(file #55857)

Nicholas Jankowski <nrjank>
Group Member
Sun 17 Mar 2024 01:48:11 AM UTC, original submission:  

I have modified the unique function to handle cellstr arrays with the rows option. The modified function (see attachment) includes two extra BISTs for the new functionality and a minor adjustment in the documentation regarding the “rows” option. I have attached a patch file 'unique.diff'.

This is an Octave extension, since MATLAB does not support this functionality. Another issue about MATLAB's unique implementation is that it does not allow a combination of 'sorted'/'stable' and 'first'/'last' options. Octave behaves the same. As a result, if we opt for 'first'/'last', these are always computed with 'sorted' by default, which is a bit annoying since there are cases that some extra flexibility would be very useful. Are we interested in obtaining this extra functionality? If so, I could work on this and submit another patch.

Andreas Bertsatos <pr0m1th3as>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #56936:  unique.m added by pr0m1th3as (15KiB - text/x-objcsrc)
file #56529:  unique_cellstr_rows_patch10440_v2.patch added by nrjank (4KiB - application/octet-stream - updated patch against default (hg id c29969c6b6ac))
file #55857:  unique_cellstr_rows_patch10440.patch added by nrjank (3KiB - application/octet-stream - patch against default (hg id ce6d586e18c4))
file #55848:  unique.diff added by pr0m1th3as (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by nrjank (Updated the item)
  • -email is unavailable- added by pr0m1th3as (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2025-02-21 pr0m1th3as Attached File- Added unique.m, #56936
        Attached File- Added unique_cellstr_row_patch.diff, #56937
    2025-02-21 nrjank Open/ClosedOpen Closed
    2025-02-21 nrjank StatusNeed Info Wont Do
    2025-02-13 pr0m1th3as Attached File- Added unique_cellstr_row_patch10440_v3.diff, #56885
    2024-10-14 nrjank Attached File- Added unique_cellstr_rows_patch10440_v2.patch, #56529
    2024-10-14 nrjank StatusIn Progress Need Info
    2024-03-17 nrjank Attached File- Added unique_cellstr_rows_patch10440.patch, #55857
        StatusNone In Progress
    2024-03-17 pr0m1th3as Attached File- Added unique.diff, #55848

    Back to the top

    Powered by Savane 3.15-e283.
    Corresponding source code