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

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

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

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.

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

The results are shown below

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

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.

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().

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.4