bugGNU Octave - Bugs: bug #52647, missing function: erase...

 
 

bug #52647: missing function: erase (introduced Matlab 2016b)

Submitted by:  Thomas A. F. Thorne <tafthorne>
Submitted on:  Tue 12 Dec 2017 01:41:59 PM UTC  
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Thomas A. F. Thorne Open/Closed:  Closed
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

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

 

( Jump to the original submission)

Wed 27 Dec 2017 07:29:33 PM UTC, comment #23: 

I added a new function isstring() which reports whether an object is a string array or not (http://hg.savannah.gnu.org/hgweb/octave/rev/0b65949870e3).  Since Octave does not implement string arrays, this will always return false.  However, it is still useful if programmers are writing for both Octave and Matlab because they can determine at run-time what is supported.

Rik <rik5>
Project Administrator
Wed 27 Dec 2017 05:00:34 PM UTC, comment #22: 

I added a warning when a programmer uses a char array for PTN instead of either a string or cell array of strings.  See http://hg.savannah.gnu.org/hgweb/octave/rev/42d099b841aa.

Rik <rik5>
Project Administrator
Tue 26 Dec 2017 11:54:21 AM UTC, comment #21: 

Rik:

  Here is a summery of what I have found.

We can use char arrays under the following conditions.
nstr=erase( “The slow cat caught the mouse” , “cat ”)
nstr=erase( “The slow cat caught the mouse” , [“cat”  ; “slow “])
nstr=erase(originalstr, delstr)

1)  if originalstr is just one string (use num2cell )
2)  if delstr is just one string then trailing spaces will work.
          (Use num2cell )
3)  if delstr is multiple strings then only  the longest one will have
        correct trailing spaces.   The rest will have too many trailing spaces.
           (Use num2cell )
      This should not be allowed because of the trailing spaces.

We can use cells for any condition.
nstr=erase( {“The slow cat caught the mouse”} , {“cat ”  ; “slow “})
nstr=erase( {“The slow cat caught the mouse”} , {“cat ”  , “slow “}) 
nstr=erase( {“The slow cat” ; “ caught the mouse”} , {“cat ”  , “slow “}) 
etc.
 (use cellstr)

Doug Stewart <dastew>
Tue 26 Dec 2017 11:51:27 AM UTC, comment #20: 

Sahil:

I think Rik's answer is correct  "Hell!"

Doug Stewart <dastew>
Tue 26 Dec 2017 11:09:01 AM UTC, comment #19: 

If somehow, we are not able to address the case highlighted by Doug, we can switch to this version (due thanks to Andy and Doug) of ours which is slightly more faster than this version.

But there's a problem with this. Test cases on lines 173,175, the erase function returns sq_string of size 0x0 and we expect it to be null_sq_string ('') . Also, one of the test cases written by Rik is wrong according to me

%!assert (erase ("Hello World!", {"o"; "World"; " "}), "Hell!")

The expected string should be "HellWrld!" instead.
All other considerations pointed by Rik in coding style have been included in this code (using STR and MATCH instead of STR1 and STR2 resp.)   

(file #42732)

Sahil <batterylow>
Tue 26 Dec 2017 05:38:02 AM UTC, comment #18: 

I'll ask someone to run it on the Maintainer's List and report the results.

Rik <rik5>
Project Administrator
Tue 26 Dec 2017 03:52:02 AM UTC, comment #17: 

I believe this does work in matlab but not octave.

Doug Stewart <dastew>
Tue 26 Dec 2017 03:48:36 AM UTC, comment #16: 

It would be helpful to run

erase('this is it',['this';'is'])

under Matlab.  Does it really produce an error and refuse to run?

Octave is a superset of Matlab so if they don't handle this input case then we are free to choose our own solution.

Rik <rik5>
Project Administrator
Tue 26 Dec 2017 03:17:57 AM UTC, comment #15: 

%!error  <erase: STR2 must be a cell array of strings or a char vector> erase ('Hello World t ',['ld '; 'o '])

Was there when I was working on it.

Matlab treats quotes very differently than octave does. I think that we cannot be 100% compatible with matlab because of this.

The best compromise that we came up with was to restrict char array, and just allow char vectors for the match section, and to warn when char arrays were used for the input str, that trailing spaces might change.

Doug Stewart <dastew>
Tue 26 Dec 2017 02:56:14 AM UTC, comment #14: 

@Doug: What is the answer from Matlab for

erase('this is it',['this';'is'])

If I had to guess, I think the pattern char matrix would be expanded to ['this'; 'is  ']).  The word 'this' will match, but 'is  ' will not, so the final answer is ' is it'.

Unless of course Matlab is trimming whitespace in patterns, in which case cellstr rather than mat2cell would be appropriate, but only for patterns.

Also, there was no BIST test for this configuration so no way to know that we had changed behavior.

Rik <rik5>
Project Administrator
Tue 26 Dec 2017 02:43:25 AM UTC, comment #13: 

@Rik
Your changes make erase give the wrong answer to this example.

 erase('this is it',['this';'is'])

Sahil and I spent hours looking int this problem, and I believe we came up with a good compromise. Then Andy took over and made changes then Rik took over and made more changes. The result was that all the careful work done was thrown out.

Doug Stewart <dastew>
Mon 25 Dec 2017 05:08:52 PM UTC, comment #12: 

I added erase to Octave in this changeset (http://hg.savannah.gnu.org/hgweb/octave/rev/892f7f096ffb).

Coding for core Octave is very hard because one simultaneously needs to have code that implements a specific functionality, that is high performance, that is well documented, and that follows coding guidelines so it can be maintained.

In order to help Sahil learn about the process I've annotated how I transformed his original changeset into the one I checked in.

First, there were some functional issues.  The original code didn't accept a character array for the second input.  Also, because the code used cellstr there could be changes in trailing whitespace in the first input.

The solution was to use mat2cell(), rather than cellstr(), to convert to a cell while preserving all strings intact.

Second, was performance.  Generally, for loops are very slow in an interpreted language.  Also, rather than re-invent the wheel it is best to re-use existing functions and wrap them with new code so that they can do whatever is required.

I wrote a small benchmarking script (tst_erase.m) to test some ideas.

N = 3e3;
bm = zeros (N,1);

#str = "the quick brown fox jumps over the lazy dog";
str = {"the quick brown fox jumps";
       "over the lazy dog"};
#matchstr = "the ";
matchstr = {"the ", "lazy "};

y = "";

for i = 1:N
  tic;
#  y = erase (str, matchstr);
#  y = strrep (str, matchstr, "");
#  y = regexprep (str, matchstr, "");

  bm(i) = toc;
endfor

sum (bm)

And in order to avoid being fooled by a single measurement, I actually ran the test script multiple times using

for j=1:5; tst_erase; bm2(j) = sum (bm); endfor; mean (bm2)

The results are shown below

################################################################################
Case # 1
str = "the quick brown fox jumps over the lazy dog"};
matchstr = "the ";
N = 3e3
############################################################
erase : 0.91 sec
strrep : 0.048 sec
regexprep : .054 sec
newerase: 0.27 sec

################################################################################
Case # 2
str = {"the quick brown fox jumps";
       "over the lazy dog"};
matchstr = "the ";
N = 3e3
############################################################
erase : 1.10 sec
strrep : 0.044 sec
regexprep : .076 sec
newerase: 0.27 sec

################################################################################
Case # 3
str = {"the quick brown fox jumps";
       "over the lazy dog"};
matchstr = {"the ", "lazy "};
N = 3e3
############################################################
erase : 1.07 sec
newerase : 1.38 sec

At first, newerase did not exist, and I was just looking at whether I could use existing functions like strrep or regexprep to implement the functionality.  As you can see, strrep is the fastest, but it doesn't handle multiple patterns in the way erase required.  That's okay, in my new version of erase I check the number of patterns and if it is 1, which it often will be, I use strrep for faster performance.  But if it is more than 1 then I fall back to regexprep because it is still faster than coding your own solution with a for loop.

The complete new code is

## Copyright (C) 2017 Sahil Yadav
##
## This file is part of Octave.
##
## Octave is free software; you can redistribute it and/or modify it
## under the terms of the GNU General Public License as published by
## the Free Software Foundation; either version 3 of the License, or
## (at your option) any later version.
##
## Octave is distributed in the hope that it will be useful, but
## WITHOUT ANY WARRANTY; without even the implied warranty of
## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
## GNU General Public License for more details.
##
## You should have received a copy of the GNU General Public License
## along with Octave; see the file COPYING.  If not, see
## <http://www.gnu.org/licenses/>.

## -*- texinfo -*-
## @deftypefn  {} {@var{newstr} =} erase (@var{str}, @var{ptn})
## Delete all occurrences of @var{ptn} within @var{str}.
##
## @var{str} and @var{ptn} can be ordinary strings, cell array of strings, or
## character arrays.
##
## Examples
##
## @example
## @group
## ## string, single pattern
## erase ("Hello World!", " World")
##     @result{} "Hello!"
##
## ## cellstr, single pattern
## erase (@{"Hello", "World!"@}, "World")
##     @result{} @{"Hello", "!"@}
##
## ## string, multiple patterns
## erase ("The Octave interpreter is fabulous", @{"interpreter ", "The "@})
##     @result{} "Octave is fabulous"
##
## ## cellstr, multiple patterns
## erase (@{"The ", "Octave interpreter ", "is fabulous"@}, @{"interpreter ", "The "@})
##     @result{} @{"", "Octave ", "is fabulous"@}
## @end group
## @end example
##
## Programming Note: @code{erase} deletes the first instance of a pattern in a
## string when there are overlapping occurrences.  For example,
##
## @example
## erase ("abababa", "aba")
##     @result{} "b"
## @end example
##
## See @code{strrep} for processing overlaps.
##
## @seealso{strrep, regexprep}
## @end deftypefn

## Author: Sahil Yadav <yadavsahil5198@gmail.com>


function newstr = erase (str, ptn)

  if (nargin != 2)
    print_usage ();
  endif

  ischarmatrix = false;
  if (ischar (str))
    if (rows (str) > 1)
      ## Convert to cell.  Can't use cellstr which trims spaces.
      str = mat2cell (str, ones (1, rows (str)));
      ischarmatrix = true;
    endif
  elseif (! iscellstr (str))
    error ("erase: STR must be a string or cell array of strings");
  endif

  if (ischar (ptn))
    if (rows (ptn) > 1)
      ## Convert to cell.  Can't use cellstr which trims spaces.
      ptn = mat2cell (ptn, ones (1, rows (ptn)));
    endif
  elseif (! iscellstr (ptn))
    error ("erase: PTN must be a string or cell array of strings");
  endif

  nptn = ifelse (ischar (ptn), 1, numel (ptn));
  if (nptn == 1)
    newstr = strrep (str, ptn, "", "overlaps", false);
  else
    ptn = regexptranslate ("escape", ptn(:).');
    ptn = strjoin (ptn, '|');
    newstr = regexprep (str, ptn, "");
  endif

  if (ischarmatrix)
    newstr = char (newstr);
  endif

endfunction


%!assert (erase ("Hello World!", " World"), "Hello!")
%!assert (erase ({"Hello World!"}, " World"), {"Hello!"})
%!assert (erase (char ("Hello", "World!"), "World"), char ("Hello ", "!"))
%!assert (erase ({"Hello", "World!"}, "World"), {"Hello", "!"})
%!assert (erase ("Hello World!", {"o"; "World"; " "}), "Hell!")

## Test overlaps
## from https://savannah.gnu.org/bugs/?52647#comment5
%!assert (erase ('ababa', 'aba'), 'ba')
%!assert (erase ('abababa', 'aba'), 'b')
%!assert (erase ('ababababa', 'aba'), 'bba')
%!assert (erase ('ababababa', {'aba', 'bba'}), 'bba')
%!assert (erase ('ababababa ', {'aba', 'bba'}), 'bba ')
%!assert (erase ({' ababababa '}, {'aba', 'bba'}), {' bba '})
%!assert (erase (' ababa ', {'aba', 'baba'}), ' ba ')
%!assert (erase (' Hello World t ', {'t';'llo'}), ' He World  ')
%!assert (erase ({' Hello World t '}, [ 'o ']), {' HellWorld t '})
%!assert (erase ( 'Hello World t ', {'ld '; 'o '}), 'HellWort ')
%!assert (erase ('aba', 'aba'), '')
%!assert (erase ({'aba'}, 'aba'), ({""}))
%!assert (erase ('', 'aba'), '')
%!assert (erase ({'abbabbabba'},{'abba'}), {'bb'})
%!assert (erase ({'ababababa';'abbabbabba'}, 'aba'), {'bba';'abbabbabba'})
%!assert (erase ({''}, {''}), {''})
%!assert (erase ({'pppppppp'}, 'p'), {''})
%!assert (erase ('Hello World t ', {'ld '; 'o '}), 'HellWort ')
%!assert (erase ({'Hello World t '}, {'ld '; 'o '}), {'HellWort '})

## Test input validation
%!error erase ()
%!error erase ("a")
%!error erase ("a", "b", "c")
%!error <STR must be a string> erase ([1], "foo")
%!error <PTN must be a string> erase ("foo", [1])

First, the GPL license was copied over correctly.  That is good, but the Copyright date was 2002-2017.  This is a new file so you can only claim copyright for 2017 and beyond.  I changed that.

I rewrote the documentation to use NEWSTR, STR, and PTN as function arguments.  These are the same names that are used in strrep().  It is like naming variables, in that one wants to be as clear as possible.  If the inputs are just STR1 and STR2 then a user is forced to read the documentation to figure out what they are.  But with STR and PTN as inputs you can sort of guess what the function is supposed to do without reading everything.

It is also important for users to know about related functions so I used the @seealso macro to point towards strrep and regexprep.

I changed the names of the variables used in the function prototype to match that in the documentation.  This is helpful because even programmers often reference the documentation to see what is supposed to happen in the function.  It is confusing when the input in the documentation is STR1 but called parentstring in the actual code.

%!demo blocks are most useful for plotting functions.  I removed them since I don't think they help the user here.  I also added %!error tests to check the input validation.  Finally, I added some more regular %!test blocks.  The existing BIST tests were focused on corner cases around overlaps, but we needed some simple general tests of functionality.

(file #42727)

Rik <rik5>
Project Administrator
Fri 22 Dec 2017 04:04:06 PM UTC, comment #11: 

A few coding style issues were there, which have been rectified now.

(file #42703)

Sahil <batterylow>
Thu 21 Dec 2017 01:20:46 PM UTC, comment #10: 

 Sahil and I have tested this code, and we now think it is ready to be used.
  Because octave uses double and single quotes differently than Matlab, we cannot get 100% compatibility, but we think that this code is quite useful as it is. Some one tying to convert from matlab to octave will see a message that will help them make the changes for it to work on octave.

 Sahil ask how to contribute to the octave code and I chose this as a starter project for him. I think he has great potential to become a real octave developer.   I have a second project for him :-)
Doug

Doug Stewart <dastew>
Thu 21 Dec 2017 01:05:15 PM UTC, comment #9: 

The code has been optimised a bit. 

(file #42694)

Sahil <batterylow>
Mon 18 Dec 2017 08:42:35 AM UTC, comment #8: 

We have unit-tested the function with the edge tests, if however, someone encounters any problem, suggestions/feedback, kindly let us know. Again, thanks to the contributors for helping me out at every step.

(file #42670)

Sahil <batterylow>
Sun 17 Dec 2017 04:03:24 AM UTC, comment #7: 

Yes we are still working on it.

Doug Stewart <dastew>
Sun 17 Dec 2017 04:00:48 AM UTC, comment #6: 

Is there a second version of erase.m coming?  That was the suggestion from comment #4.

Rik <rik5>
Project Administrator
Thu 14 Dec 2017 05:16:39 PM UTC, comment #5: 

I am providing here examples from 2017b for the purpose of test units. They mainly cover the logic for trailing whitespace (must be kept) and for overlapping matches.

> erase ('aba', 'aba')

ans =

  0x0 empty char array

>> erase ('ababa', 'aba')

ans =

    'ba'

>> erase ('abababa', 'aba')

ans =

    'b'

>> erase ('ababababa', 'aba')

ans =

    'bba'

>> erase ('ababababa', {'aba', 'bba'})

ans =

    'bba'

>> erase (' ababababa ', {'aba', 'bba'})

ans =

    ' bba '

>> erase (' ababa ', {'aba', 'baba'})

ans =

    ' ba '

Carnë Draug <carandraug>
Project Member
Wed 13 Dec 2017 03:37:27 PM UTC, comment #4: 

I have suggested some improvements to Sahil, so an improved version is on its way.

Doug Stewart <dastew>
Wed 13 Dec 2017 01:15:21 PM UTC, comment #3: 

I tried my best not to fail in any of the edge cases but if so happens, kindly let me know. Any other criticisms are welcomed as well. Thanks to 'doug_' and all others for helping me out. Further guidelines as to how can i test on standard unit tests will be appreciated.

Sahil <batterylow>
Tue 12 Dec 2017 04:29:21 PM UTC, comment #2: 

There is a beginner who is starting by writing this as a M function. I will see how he makes out.

Doug Stewart <dastew>
Tue 12 Dec 2017 04:23:30 PM UTC, comment #1: 

Confirmed.  It often isn't worth it for Octave to follow Matlab too closely because they have a habit of changing their function names, discontinuing functions, altering the API, etc.  If the function survives for a few years without change then it can be worth adopting.

In the mean time, you can have the same functionality with regexprep().

regexprep (string, substring, '')

Rik <rik5>
Project Administrator
Tue 12 Dec 2017 01:41:59 PM UTC, original submission:  

A new function was introduced in MATLAB R2016b that erases sub-strings within a string. 

erase(string,sub-string)

with the return being the remains of the original string. 

Full documentation available at:
https://uk.mathworks.com/help/matlab/ref/erase.html

Thomas A. F. Thorne <tafthorne>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #42732:  erase.m added by batterylow (6KiB - text/x-objcsrc - final version-3)
file #42727:  tst_erase.m added by rik5 (362B - text/x-matlab)
file #42703:  erase.m added by batterylow (5KiB - text/x-objcsrc - in accordance with octave style guide)
file #42694:  erase.m added by batterylow (5KiB - text/x-objcsrc - final version -2)
file #42670:  erase.m added by batterylow (7KiB - text/x-objcsrc - final version till date)
file #42637:  erase.m added by batterylow (2KiB - text/x-objcsrc - I tried my best not to fail in any of the edge cases but if so happens, kindly let me know. Any other criticisms are welcomed as well. Thanks to 'doug_' for helping me out. Further guidelines as to how can i test on standard unit tests will be appreciated)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-12-26 batterylow Attached File- => Added erase.m, #42732
    2017-12-25 rik5 Attached File- => Added tst_erase.m, #42727
        StatusConfirmed => Fixed
        Open/ClosedOpen => Closed
    2017-12-22 batterylow Attached File- => Added erase.m, #42703
    2017-12-21 batterylow Attached File- => Added erase.m, #42694
    2017-12-18 batterylow Attached File- => Added erase.m, #42670
    2017-12-13 batterylow Attached File- => Added erase.m, #42637
        Carbon-Copy- => Added batterylow
    2017-12-12 rik5 Operating SystemGNU/Linux => Any
    2017-12-12 rik5 StatusNone => Confirmed
        Release4.0.0 => dev

    Back to the top


    Powered by Savane 3.5