bugGNU Octave - Bugs: bug #57867, strrep: unexpected substitution...

 
 

bug #57867: strrep: unexpected substitution behavior on a char array

Submitter:  Nicholas Jankowski <nrjank>
Submitted:  Thu 20 Feb 2020 06:13:21 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Nicholas R Jankowski Open/Closed:  * Closed
Release:  * dev Release: 
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 14 Sep 2020 11:37:12 PM UTC, comment #69: 

Adding the expanded behavior to operate on char arrays was not as simple as expected.  I checked in a change which does that here: http://hg.savannah.gnu.org/hgweb/octave/rev/7dcc4cb6da53.  I also re-wrote some of the existing strrep to be clearer.

Marking bug as fixed and closing report.

Rik <rik5>
Group administrator
Thu 25 Jun 2020 03:15:13 PM UTC, comment #68: 

What's the problem? if anything wrong? 10 days till now, and no one reply.. if something wrong?

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Mon 15 Jun 2020 01:31:59 AM UTC, comment #67: 

Firstly, I sware that's by pure chance. If there is any problem I will change it immediately. Secondly, I preferred to make the help text as the final step because I don't know the decision of developers and maintainers about my new additions yet. Finally thank you for everything.

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sun 14 Jun 2020 08:19:59 PM UTC, comment #66: 

Ok, thanks. I do see the test you added and your test/build results.  The patch does push cleanly onto the latest source code, but I can't verify build and function. will need to wait for someone else there. 

Meanwhile, do you think we should make any changes to the help text for the function to call out that it can be used on a char array, and how it will behave?  Last, I don't know how much the maintainers would or would not care that the output of the test (shit) is considered a mild but derogatory english word.

Nicholas Jankowski <nrjank>
Group Member
Sun 14 Jun 2020 07:47:42 PM UTC, comment #65: 

This is a new patch after adding the test for my addition to the source code and also there are three screenshots, the first for octave, using a test build version with the corrected bug, and the second for successful testing of old tests which are 19 tests and the last one for successful testing after adding a new test for my addition so in this case, the total tests become 20.


(file #49266,

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sun 14 Jun 2020 02:53:11 AM UTC, comment #64: 

Did you verify that this patch applies successfully to the latest source code?

Did you add built in tests to verify that it both fixes the bug? Did you verify that it still passes all the old tests?

Nicholas Jankowski <nrjank>
Group Member
Sun 14 Jun 2020 01:43:41 AM UTC, comment #63: 

Here, I am again. I try again to make a successful patch. check it, please.

(file #49256)

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Mon 11 May 2020 04:00:54 PM UTC, comment #62: 

First, I notice that this patch still didn't add any tests to verify the bugfix.  But more importantly, that patch doesn't even apply to my copy of the source code:


$ hg patch --no-commit ../mypatch8.patch
applying ../mypatch8.patch
patching file libinterp/corefcn/strfind.cc
Hunk #1 FAILED at 441
Hunk #2 FAILED at 469
2 out of 2 hunks FAILED -- saving rejects to file libinterp/corefcn/strfind.cc.rej
abort: patch failed to apply


I'm noticing some leading whitespace issues in the first line to be removed deleted. (patch line 16)  That whitespace isn't there in strfind or in your previous patch. 

and your 2nd hunk (line 47) is showing line 469 (starting with retval) rather than the 470 (starting with else). I would have thought that would have applied with a 1 line offset, not sure why it failed outright. But it doesn't look like anyone else has made a change to strfind.cc in between your two patches. 

Are you manually editing the patch file? Or making changes not included in your patch? I'm not sure how else you would get those type of changes.



Nicholas Jankowski <nrjank>
Group Member
Mon 11 May 2020 02:55:30 PM UTC, comment #61: 

I hope that is the correct one. The patch which is called ("mypatch8")

(file #49057)

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Thu 07 May 2020 06:24:33 PM UTC, comment #60: 

I don't know what to tell you. You attached a file 'strrep.diff' on April 21, which looks like a proper Mercurial commit export.

When I apply that patch to the Octave default branch, the patch applies cleanly. But I get the following when I try to build it


../libinterp/corefcn/strfind.cc: In function ‘octave_value_list Fstrrep(const octave_value_list&, int)’:
../libinterp/corefcn/strfind.cc:457:5: error: expected ‘}’ before ‘else’
  457 |     else if (argstr.iscell ())
      |     ^~~~
../libinterp/corefcn/strfind.cc:445:3: note: to match this ‘{’
  445 |   {
      |   ^
../libinterp/corefcn/strfind.cc:430:16: warning: control reaches end of non-void function [-Wreturn-type]
  430 |   octave_value retval;
      |                ^~~~~~


If you have made changes that build for you, then maybe you should check that the latest patch file here actually matches the changes you intended.

Mike Miller <mtmiller>
Group Member
Fri 01 May 2020 02:25:05 AM UTC, comment #59: 

I don't understand what's wrong, I build the octave from source and the code works well..strrep function makes the required... I uploaded screenshots with what I have said
the screens show: - make check output
                  - ./run-octave


Abdelrhman_Adel_Moawad <abdelrhman_adel>
Wed 29 Apr 2020 06:06:03 PM UTC, comment #58: 

The latest patch does not even compile for me, so that's not a good start. In addition the coding style is still very wrong, and does not add any tests for the new behavior.

Mike Miller <mtmiller>
Group Member
Mon 27 Apr 2020 11:40:25 AM UTC, comment #57: 

So, is the updated function once again compatible?

Did you add any other tests for the new behavior? Did you leave the other tests unchanged?

Does the new function pass all tests?

Nicholas Jankowski <nrjank>
Group Member
Mon 27 Apr 2020 09:27:43 AM UTC, comment #56: 

anything wrong?

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Wed 22 Apr 2020 07:32:18 PM UTC, comment #55: 

Ok, I will describe the main problem and how I think to fix it and what I have done in the source code.
The Problem:
the substitution of character array in 2D-array .. the sequence of the characters will be confused for example:

> a=["What is this ?"; "How is this ?"] 
> a=

  What is this?
  How is this?
if we use that command: a(:), the answer will be...
W H h o a w t  i i s s  t t h h i i s s
so when we use strrep function, it make nothing as it can't find the expected sequence

> strrep(a,"is", "does")
> ans=

  What is this ?
  How is this ?
but cell string works well so if we treat char array as cell string by change the if(input.is_string()) condition behavior to convert input to cell string by: input' =input.cell_str()
and treat input' to make replacement then use char(input',input'.numel()) to convert back to char array to make output compatible.
and this only in case: if(input.is_string()).
The only change in the test will be:
from: %!assert (size (strrep ("a", "a", "")), [0 0])
to: %!assert (size (strrep ("a", "a", "")), [1 0])
So the result of all that is:

> strrep(a,"is","does")
> ans=

     What does this?
     How does this?

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Wed 22 Apr 2020 02:12:38 AM UTC, comment #54: 

yes, it appears you are now following the correct procedures to produce a patch from the latest version of the source.

the other comments were about the actual function:

- adherence to coding style
- the behavior of the function to maintain compatibility
- Mike's comments about what the output of the function should be.
- comments about the new function still passing the tests for behavior that should still occur.
- comments about new tests to ensure the new patch performs the new function.

In general, you should never upload a file without a Discussion Comment up here describing what was done. this should be a conversation about the work being done. It's easier to stay on an agreed path and avoid wasting effort that way.

Nicholas Jankowski <nrjank>
Group Member
Tue 21 Apr 2020 03:11:59 PM UTC, comment #53: 

I use ubuntu terminal to :
1) hg clone http://www.octave.org/hg/octave octave
2) cd octave/libinterp/corefcn
3) nano strfind.cc (make change)
4) hg up
5) hg commit -m (commit message)
6) hg export -o strrep.diff tip
these are all the steps that I had used then I uploaded the patch exported here.
what's wrong, please??

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Tue 21 Apr 2020 03:06:23 PM UTC, comment #52: 

do you mean ... write how I follow the links in comments 46, 47
to make the last version patch ??

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Tue 21 Apr 2020 02:33:27 PM UTC, comment #51: 

could you comment here on how this latest version addresses mike's concerns in comment #46 and mine in comment #47?

Nicholas Jankowski <nrjank>
Group Member
Tue 21 Apr 2020 02:28:46 PM UTC, comment #50: 

I upload a patch file with changes ... I do my best and follow instructions to make the patch more clean and perfect

(file #48894)

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sun 19 Apr 2020 12:21:21 PM UTC, comment #49: 

yes, I will continue, I know that I disappeared the previous weeks but, I will try again and write here soon

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sun 12 Apr 2020 06:40:20 PM UTC, comment #48: 

I realize it's been about two weeks since the last update here after what was a relatively fast pace for a while. I realize that also aligned with the GSOC proposal deadline.  Do you still want to try to complete this bugfix?

Nicholas Jankowski <nrjank>
Group Member
Mon 30 Mar 2020 02:14:13 AM UTC, comment #47: 

sorry, i didn't look closely enough at the Final2.patch.

you should never remove tests from previous versions of the code without very good reason. they exist largely to make sure that future code changes don't break necessary function.  in this case, returning a cell for all cases breaks both backward and matlab compatibility.

Please see the information at
https://wiki.octave.org/Contribution_guidelines

It links to the section on using Mercurial to generate patches off the sourcecode, on building Octave, and on Coding style.

The step by step instructions for making a patch once you've cloned the source is at:
https://wiki.octave.org/Mercurial#Creating_changesets_files_with_hg

After taking care of the issues Mike mentioned, edit the strfind.cc file where it is (at /libinterp/corefcn/strfind.cc), don't move it. then make a commit and export the patch.

Nicholas Jankowski <nrjank>
Group Member
Sun 29 Mar 2020 09:09:08 PM UTC, comment #46: 

This change doesn't look good to me at all.

First of all, the patch doesn't apply cleanly when applied to the top of the Octave repository. It would be really helpful if you could learn how to produce a proper patch or Mercurial exported changeset against the Octave repository. Can you maybe explain the difficulty you are having working with Mercurial to produce such a patch?

With some small effort, I was able to apply the patch to the default branch. However, the changes do not adhere to the GNU or Octave coding styles, for example mixing tabs and spaces and incorrect indentation.

The new behavior is also very unsatisfactory. You have changed the behavior of this function so that it returns a cell array always, even when the input is a normal char row vector. That is not desirable.

Also, I know we are extending Matlab behavior here, so what the function should do is debatable, but I think it would be wrong to return a cell array when the input is a char matrix. My intuition is if the input is a char row vector, the output should be the same. When the input is a cell array, the output should be the same. So when the input is a char matrix, the output should probably also be a char matrix, with rows space-padded or truncated as needed.

Mike Miller <mtmiller>
Group Member
Sun 29 Mar 2020 11:41:34 AM UTC, comment #45: 


> Now, If there any step that should I do else?


at this point I think it's best to just wait for a developer to have time to review the patch.

Nicholas Jankowski <nrjank>
Group Member
Sun 29 Mar 2020 02:53:22 AM UTC, comment #44: 

I am being grateful for your help sir.
Now, If there any step that should I do else?

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sun 29 Mar 2020 02:07:02 AM UTC, comment #43: 

since you also changed the help text you should verify that

>> help strrep


also looks okay.

Other than maybe coming up with any other ways to test the new input, I think this looks ready for a primary developer to check.

thanks for sticking with this one.

Nicholas Jankowski <nrjank>
Group Member
Sun 29 Mar 2020 12:30:30 AM UTC, comment #42: 

what can I do else?

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sat 28 Mar 2020 09:13:36 PM UTC, comment #41: 

a copy from the octave terminal :
octave:1> c=["Hello World";"Goodbye World"]
c =

Hello World 
Goodbye World

octave:2> strrep(c,'World','Jane')
ans =
{
  [1,1] = Hello Jane 
  [2,1] = Goodbye Jane
}

octave:3> cd ..
octave:4> cd libinterp/corefcn
octave:5> test strfind.cc
PASSES 19 out of 19 tests
octave:6>

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sat 28 Mar 2020 09:08:16 PM UTC, comment #40: 

I attached a modified patch file.
This error (sttrrep: each element of S must be a string) has been deleted as it's a mistake from me.
I modified the test cases to be suitable for the changed code.
The commit message has been modified too.

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sat 28 Mar 2020 06:29:36 PM UTC, comment #39: 

thanks, I can't verify that it works but that looks much better.

I would recommend adding an error test for the error condition you added (sttrrep: each element of S must be a string). You should use the form that checks for the correct error message, shown here:
https://wiki.octave.org/Tests#Error_.2F_Warning


Also, the commit message is still not following the guidelines at:
https://wiki.octave.org/Commit_message_guidelines

"maint" is for "for reorganisation of the sources that do not change the source". 

a 1-file change can probably just have a 1-line commit message (since this doesn't change any of the documentation or the NEWS file).  so it should start with the filename, mention the function and fix, and end in the bug number.  something like:


strfind.cc (strrep): char array inputs now parsed as cell arrays (bug #57867)


Nicholas Jankowski <nrjank>
Group Member
Sat 28 Mar 2020 04:34:00 PM UTC, comment #38: 

I uploaded the final merged patch with 1)code change
                                       2)test cases(%!assert..)

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sat 28 Mar 2020 06:54:53 AM UTC, comment #37: 

Finally, I guess I did it.
I make the test for my changeset and it succeeded.
I attached two patches one by the code change and the other by test
I also have a screen from the ./run-octave in the terminal.
Thanks for all of your help.
you can review it now.

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Fri 27 Mar 2020 05:15:52 PM UTC, comment #36: 

Yes, that's all correct, verifying that a patch works and running tests against it means that you need to compile Octave and run the tests in the built Octave.

Mike Miller <mtmiller>
Group Member
Wed 25 Mar 2020 11:44:13 PM UTC, comment #35: 

I do everything to solve the problem but useless...
when I use ./configure cmd .. gives:  error: BLAS and LAPACK libraries are required
if the configuration is done successfully I think using 'make' will  build the octave
but I can't solve it

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Wed 25 Mar 2020 03:26:25 AM UTC, comment #34: 

@Mike, correct me if I'm wrong here since I don't usually work on the compiled code:

information about tests can be found in the manual [1] and the wiki [2]:

in particular, to run a test on any m file, within octave you can just type:

>> test mfilename.m


it'll run the test on the file either in the local folder or in the loadpath.

for tests in a compiled (.cc) file, you will need to compile octave with the new code, then run the test on the .cc file:

>> cd /path/to/source/octave/libinterp/corefcn
>> test ("strfind.cc")


if you run the test with just an installed version of Octave (not compiled with your changes) it'll run the test with the installed version of strfind.  for example, if I run the test on strfind now with v5.2.0, it fails because there is a test for the new "forcecelloutput" option, which is in the source code for release with version 6. It would similarly not show any effect from changes you're making to fix this bug.

If you have not compiled Octave from source to run these tests, and need help compiling after looking through the instructions [3], you should ask at the help mailing list help@octave.org. 


[1] https://octave.org/doc/interpreter/Test-Functions.html

[2] https://wiki.octave.org/Tests#Running_tests

[3] https://wiki.octave.org/Summer_of_Code_-_Getting_Started#Things_You.27ll_be_Expected_to_Know_or_Quickly_Learn_On_Your_Own

Nicholas Jankowski <nrjank>
Group Member
Wed 25 Mar 2020 01:16:03 AM UTC, comment #33: 

The only thing which is unclear for me is the way of testing the change in the source code if it's useful for the bug solution or not? please, anyone, tell me what should I do ?

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Tue 24 Mar 2020 10:49:08 PM UTC, comment #32: 

or I attached the patch and it will be reviewed ?

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Tue 24 Mar 2020 09:50:11 PM UTC, comment #31: 

I have a question please, How can I test my code that it works as I want?
when I put a test case at the end of strfind.cc which I make the change in it  ... and try

>> test strfind.cc

after putting its path it gives me fail the test and doesn't recognize any changes !!

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Tue 24 Mar 2020 07:10:07 PM UTC, comment #30: 

The attached file isn't really a proper patch file, it looks like a copy-paste from a terminal reading a patch file through a pager. If you want to generate a patch file, please use 'hg export` properly, this is all described on the wiki.

If there is a big enough change in behavior, then yeah it should probably be documented, and tests definitely should be added.

One complication to add to comment #29, running tests on built in functions is a little trickier for now. For this function, it would be something like `test libinterp/corefcn/strfind.cc-tst`, or maybe `test /full/path/to/libinterp/corefcn/strfind.cc`.

Mike Miller <mtmiller>
Group Member
Tue 24 Mar 2020 01:35:02 AM UTC, comment #29: 

thank you for that patch file.  that is definitely a step in the right direction.  Please continue to use patches like that to contribute changes.

some thoughts:

- should there be any changes to the help text so the user understands any changed behavior

- you should add a few self tests to the function that verify your new code produces the correct output.  (these are the lines at the end of the file starting with %!assert and %!error, so that the function can be tested with

>> test strrep


and to verify that none of the other functions have been broken.

- if no other files need to be edited, you could follow the 1-line commit message guidelines from
https://wiki.octave.org/Commit_message_guidelines

Nicholas Jankowski <nrjank>
Group Member
Mon 23 Mar 2020 07:03:16 PM UTC, comment #28: 

please ATTACH the patch file to this bug report just like you attached the other files.

BUT FIRST, edit it to use a much shorter commit message.  the commit message should be brief, and should follow the guidelines linked in comment #3.  a very brief description of what was changed, the bug number, and the files edited. 

follow the instructions. then attach the patch file please.

Nicholas Jankowski <nrjank>
Group Member
Mon 23 Mar 2020 06:58:52 PM UTC, comment #27: 

I make a patch with the changeset that I made to strfind.cc after hg clone the it's source code the patch(#9912)...I wish that I have reached the required

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Mon 23 Mar 2020 03:17:12 AM UTC, comment #26: 

ok.  we've been asking for the same thing since comment #3.

Nicholas Jankowski <nrjank>
Group Member
Mon 23 Mar 2020 01:19:34 AM UTC, comment #25: 

I apologize .. I upload the file before reading the comment #22
I will do so.

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Mon 23 Mar 2020 01:16:13 AM UTC, comment #24: 

please stop making diff patches that don't apply to the Octave code base

Nicholas Jankowski <nrjank>
Group Member
Mon 23 Mar 2020 12:26:03 AM UTC, comment #23: 

I attach a patch file by mu new function but till now can't make my bookmark visible in octave repo

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Mon 23 Mar 2020 12:22:38 AM UTC, comment #22: 

Simple question:  do you know how to clone the octave source code, make edits to the octave source code, and create a patch with those edits?

If you know how to do so, why have you yet to propose a patch as an edit of the existing strfind.cc funciton. It seems like you're spending a lot of time on things that are not productive to solving the bug or teaching you to work with the Octave codebase.

If you have been unable to do so, please ask for help at help@octave.org.  I don't think it would be useful for you to keep proposing things here that don't follow the basics of contributing to Octave.


e.g., the code below does not need to be it's own function. maybe it would be useful inside the existing strrep function so that strrep handles different inputs on different code paths. But there is no need for that to be it's own function.

Nicholas Jankowski <nrjank>
Group Member
Sun 22 Mar 2020 08:37:37 PM UTC, comment #21: 

I make a new function which checks if the input is a char array or not
if it is...it will convert it to cell string

The function :
function retval=char_Array_to_CellStr(x)
%!   if(ischar(x(1)))
%!    retval=cellstr(x);
%!   endif
%! endfunction
%!assert (strrep (@()char_Array_to_CellStr('abababc'), "abab", "xyz"), "xyzxyzc")

I couldn't run the whole code but I ran the function itself and it worked

I think the whole code will be :

## DO NOT EDIT!  Generated automatically from strfind.cc by Make.
%! function retval=char_Array_to_CellStr(x)
%!   if(ischar(x(1)))
%!    retval=cellstr(x);
%!   endif
%! endfunction
%!assert (strfind ("abababa", "aba"), [1, 3, 5])
%!assert (strfind ("abababa", "aba", "overlaps", false), [1, 5])
%!assert (strfind ({"abababa", "bla", "bla"}, "a"), {[1, 3, 5, 7], 3, 3})
%!assert (strfind ("Linux is user-friendly. It just isn't ignorant-friendly or idiot-friendly.", "friendly"), [17, 50, 68])
%!assert (strfind ("abc", ""), [])
%!assert (strfind ("abc", {"", "b", ""}), {[], 2, []})
%!assert (strfind ({"abc", "def"}, ""), {[], []})
%!error strfind ()
%!error strfind ("foo", "bar", 1)
%!error <unknown option: foobar> strfind ("foo", 100, "foobar", 1)
%!error <each element of CELLSTR must be a string> strfind ({"A", 1}, "foo")
%!error <first argument must be a string> strfind (100, "foo")
%!error <PATTERN must be a string> strfind ("foo", 100)
%!assert (strrep ("This is a test string", "is", "&%$"),
%!                "Th&%$ &%$ a test string")
%!assert (strrep (@()char_Array_to_CellStr('abababc'), "abab", "xyz"), "xyzxyzc")
%!assert (strrep ("abababc", "abab", "xyz"), "xyzxyzc")
%!assert (strrep ("abababc", "abab", "xyz", "overlaps", false), "xyzabc")
%!assert (size (strrep ("a", "a", "")), [0 0])
%!error strrep ()
%!error strrep ("foo", "bar", 3, 4)

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sun 22 Mar 2020 12:20:50 AM UTC, comment #20: 

how does that patch apply to the current octave codebase? a patch should modify the existing strfind.cc.  this bug shouldn't  require any new files.  please look at some other bug reports to see examples of what we're talking about.  At the moment it doesn't appear you understand how to work with the octave source code and patch existing functions.

here's an example from not too long ago that involved a patch to a .cc file and a patch as a changeset:
bug #57456

see the details of the patch here, where it only made minimal changes to the existing file to correct the bug:
https://hg.savannah.gnu.org/hgweb/octave/rev/a077eadd1325

Nicholas Jankowski <nrjank>
Group Member
Sat 21 Mar 2020 10:25:37 PM UTC, comment #19: 

the .cc extension of patch and I add the problem fix report at the beginning of file and the file attached too.
https://github.com/abdelrhman323/gittest/commit/ff375cf460613d02db377b149b0887eac3d72c9c

(file #48638)

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sat 21 Mar 2020 09:48:46 PM UTC, comment #18: 

I am sorry for any mistakes ...I make a patch file by the source code with changes I had made for strrep function
the link on git-hub :
https://github.com/abdelrhman323/gittest/blob/master/patch.txt
and the file of the patch also will be attached
please, tell me if there are any mistakes
Thanks in advance

(file #48637)

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sat 21 Mar 2020 07:54:18 PM UTC, comment #17: 

Please attach a patch or hg export to this bug report, not a new oct file or a swig wrapper or standalone program. The patch should apply to a current version of Octave's default or stable branch.

I'm deleting all the attachments here that look like they are attempting to create a new strrep function from scratch using C++, STL, and swig, they are not addressing this bug report at all.

Mike Miller <mtmiller>
Group Member
Sat 21 Mar 2020 03:12:18 PM UTC, comment #16: 

this is the link of patch
https://savannah.gnu.org/patch/?9910

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Sat 21 Mar 2020 02:17:11 PM UTC, comment #15: 

I submit a patch by the code but no one reply till now
I increase my effort and make a mex file from my code .. upload it or what?

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Fri 20 Mar 2020 07:36:32 PM UTC, comment #14: 

The function with the bug is in a c++ file. the best way is to produce a patch for that file.  I guess for demonstration you could try to make an m-file program that shows the same thing, but that seems like quite a workaround. If you're set up to patch, compile, and test, you could just make the fix, and demonstrate the input/output improvement.

Nicholas Jankowski <nrjank>
Group Member
Fri 20 Mar 2020 07:00:45 PM UTC, comment #13: 

Ok, Sir... I will do that (submit a patch with the code change).
But if there is any method to convert my source code change to file.m to see the result or what?

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Fri 20 Mar 2020 05:49:28 PM UTC, comment #12: 

I'm personally not set up to work with compiled code (i stick to m-file editing). so someone else will have to try this out and see if it fixes the bug satisfactorily. Maybe @mtmiller can make a more useful comment.

I would suggest making and posting a patch with the proposed changes that maintain matlab compatibility while fixing the bug as described in the first few comments and the linked mailing list discussion.  that patch is more likely to be evaluated by other developers who can quickly look at it, push it on their system and test it out.

Nicholas Jankowski <nrjank>
Group Member
Fri 20 Mar 2020 03:33:56 PM UTC, comment #11: 

If there is anything wrong?... I am waiting for your reply to do next step .?

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Thu 19 Mar 2020 07:05:25 AM UTC, comment #10: 

I am being very grateful for your interest and guidance, of course, I am familiar with octave I used it many times in my college assignments and project and I observe bugs but I used c++ handmade functions to make the difference.
Now, I reached to the source code and study it well .. and I became to the problem issue which lies in the way of storing the character array  ['..'] despite the correctness if we use cell string {..}, so when I have an idea to handle this issue which is changing this
{****
if (argstr.is_string ())
 retval = qs_replace (argstr.char_array_value (), pat, rep,table, overlaps);
**}
                     to be...
{*************
if (argstr.is_string ()){
          retval = qs_replace (argstr.char_array_value (), pat, rep, table, overlaps);
  if(argstr==retval){
const Array<char> str = argstr.char_array_value ();
  const Array<char> a;
  const Array<char> b;
  octave_idx_type np = argstr.numel (); 
  for (octave_idx_type i = 0; i < ns; i+=2)
  { a =str[i];
    b =str[i+1];
  }
std::string x = a.string_value ();
std::string y = b.string_value ();
std::string z =x+y;
retval = qs_replace (z.char_array_value (), pat, rep, table, overlaps);
************}
so we can make the long confused character array to two right character arrays then convert them into two strings then add the two string to be one string and make the replacement by(qs_replace)function.
I wait for your useful opinion excitedly. 

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Thu 19 Mar 2020 12:33:56 AM UTC, comment #9: 

thanks. I realized when posting my last comment that the Post a Comment box is actually hidden by default, and it might not be obvious to everyone how to contribute to the discussion. This summer we've had 3-4 people post files to a bug without commentary, and thus far they all turned into frustrating learning experiences.  Glad you're sticking with this one.

This bug is to correct a function that already exists. So the correction would likely be a modification to the existing function.  So:

Are you at all familiar with Octave? Do you have the program installed?

Have you looked at the Source code? Have you cloned the source code in Mercurial and are you able to set things up to build Octave from source?

The function in question (strrep) is a function from the file strfind.cc.  inside of octave you can find the location of any function using the 'which' command:


>> which strrep
'strrep' is a built-in function from the file libinterp/corefcn/strfind.cc


Inside the source tree, you can then find that file and make changes to the source file directly. We use Mercurial so that changesets / patches can be generated by your changes, we post those to the bug tracker, and then others can apply and test out your patch. Primary maintainers of the source code can then decide when the patch is ready to be applied.

The primary concern I see with your changes thus far is that they make more than the minimal changes required to fix the bug. Octave strives to have Matlab compatible code, and because there already exists a matlab function strrep, we do not want to split it out into two functions and change the calling syntax.

Some of the development rules:

- strive for Matlab compatibility, and treat incompatibility as a bug unless you have a really good reason not to do so.
- you can use the public facing Matlab documentation to set expected behavior, and can request some input/output compatibility tests, but for copyright reasons should never look at the source code of matlab functions. Contributions can be disallowed for that reason alone.
- see links in comment #3 for guidelines on code contribution.

Nicholas Jankowski <nrjank>
Group Member
Wed 18 Mar 2020 11:31:40 PM UTC, comment #8: 

Firstly,I am very sorry about that mistake of writting in the file description .. I am a new user for this page, Seconldy, I have a question>> Do I write a code for the bug only which will be added by the developers later if confirmed or .. Impelement the functin from scratch or there is a source file for the function which I should use it ? ..
Thanks in advance

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Wed 18 Mar 2020 08:33:57 PM UTC, comment #7: 

@abdelrhman_adel:  Please do not add long descriptions to the file comment box.  Please use the discussion comment box at eth top of the page.  You will see a "Post a Comment" option with a text box that your can enter comments into. 

Reading the notes on the files, your proposed solution is not compatible with Octave. You are proposing splitting the function into two separate functions that would break Matlab compatibility.

Proposed changes should be posted as Mercurial changesets. You should also discuss the proposed changes in the Discussion section before uploading any more files.

Nicholas Jankowski <nrjank>
Group Member
Mon 16 Mar 2020 01:35:14 PM UTC, comment #6: 

@abdelrhman_adel: once again, files without any commentary.  the strrep function is part of libinterp/corefcn/strfind.cc

how do you propose your separate cc files be intergrate in a matlab-compatible way to improve the strrep function?

Nicholas Jankowski <nrjank>
Group Member
Mon 16 Mar 2020 04:57:50 AM UTC, comment #5: 


comment #3:

> @abdelrhman_adel - please add some narrative to your file upload. Does this address the bug, how so, and does it take into account the other comments below and the previous mailing list discussion? 
>
> this is a development community, please participate.
>
> In addition, code contributions are best made using patches that the maintainers can apply direct to the source code. Please see a process description here:
>
> https://wiki.octave.org/Mercurial
> https://wiki.octave.org/Category:Development
> https://wiki.octave.org/Commit_message_guidelines
>


(file #48600, file #48601)

Abdelrhman_Adel_Moawad <abdelrhman_adel>
Tue 10 Mar 2020 10:51:54 PM UTC, comment #4: 

The attached file #48571 has nothing to do with Octave or this bug report, so I'm deleting it.

Mike Miller <mtmiller>
Group Member
Tue 10 Mar 2020 04:35:11 PM UTC, comment #3: 

@abdelrhman_adel - please add some narrative to your file upload. Does this address the bug, how so, and does it take into account the other comments below and the previous mailing list discussion? 

this is a development community, please participate.

In addition, code contributions are best made using patches that the maintainers can apply direct to the source code. Please see a process description here:

https://wiki.octave.org/Mercurial
https://wiki.octave.org/Category:Development
https://wiki.octave.org/Commit_message_guidelines

Nicholas Jankowski <nrjank>
Group Member
Thu 20 Feb 2020 07:42:13 PM UTC, comment #2: 

unknown, but for reference this function does not preserve length (and as in the first examples, at any change it doesn't even try to preserve shape):


octave:7> strrep(c,'2','0000')
ans = 14  00005  36



Also, just to keep things connected a separate mailing list discussion is polling about possible options, including whether to throw an error like matlab, or keep the extended function and perform strrep operation by handling it internally as a cell array.

[2] https://octave.1599824.n4.nabble.com/Re-character-matrix-inputs-to-string-functions-tp4695818.html

Nicholas Jankowski <nrjank>
Group Member
Thu 20 Feb 2020 07:05:14 PM UTC, comment #1: 

There are several Octave functions that operate on 2-D char arrays where strings are normally expected.

Some of those functions, like 'strrep', may operate on the entire array and return a result of the same size, maybe in the same shape or in a flattened shape.

Some of those functions, like 'help', silently truncate the char array and operate only on the first row as if it were a string.


s = ['abs'; 'def'];
help (s)


See bug #49536 for the latter type of behavior.

Are there other functions with behavior like 'strrep', so that we can categorize these and make broader decisions? Other 'str*' functions?

Mike Miller <mtmiller>
Group Member
Thu 20 Feb 2020 06:13:21 PM UTC, original submission:  

as discussed on the mailing list [1], when strrep is passed a character array, as opposed to a single char vector, substitution proceeds columnwise, which is somewhat counterintuitive.

E.g., as expected:

octave:5> c = ['1 2 3 4 5 6']
c = 1 2 3 4 5 6

octave:6> strrep(c,'1 2', '6 7')
ans = 6 7 3 4 5 6


char array:
octave:7> c = ['1 2 3';'4 5 6']
c =

1 2 3
4 5 6

octave:8> strrep(c,'1 2', '6 7')
ans =

1 2 3
4 5 6

octave:9> strrep(c,'14', '67')
ans = 67  25  36
-verbatim-



note that matlab throws an error for a non-vector char array. any arrays must be string (class) arrays, or cell arrays, on which each string handled as expected.  Octave doesn't yet have a string class, but it handles cell arrays compatibly.

This behavior on char arrays doesn't appear to be documented anywhere.  Unclear if it's an intended extension beyond matlab, in which case it should probably be documented better, or should we have this function throw an error for non-vector char arrays?


[1] https://octave.1599824.n4.nabble.com/Re-line-continuations-tp4695788.html

Nicholas Jankowski <nrjank>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #49266:  bug57867modified.patch added by abdelrhman_adel (2KiB - application/octet-stream)
file #49256:  bug57867.patch added by abdelrhman_adel (1KiB - application/octet-stream)
file #49057:  mypatch8.patch added by abdelrhman_adel (2KiB - text/x-patch)
file #48894:  strrep.diff added by abdelrhman_adel (2KiB - text/x-patch)
file #48695:  Final2.patch added by abdelrhman_adel (2KiB - application/octet-stream)
file #48692:  Final.patch added by abdelrhman_adel (2KiB - application/octet-stream)
file #48654:  strrep.patch added by abdelrhman_adel (936B - text/x-patch)
file #48638:  patch.cc added by abdelrhman_adel (18KiB - text/plain)
file #48637:  patch.txt added by abdelrhman_adel (16KiB - text/plain)

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by abdelrhman_adel (Updated the item)
  • -email is unavailable- added by nrjank (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 group members can vote.

     

    Follow 42 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-09-14 rik5 Item GroupNone Feature Request
        StatusNone Fixed
        Open/ClosedOpen Closed
        Releaseother dev
    2020-06-14 abdelrhman_adel Attached File- Added bug57867modified.patch, #49266
        Attached File- Added Screenshot@from@2020-06-14@20-34-21.png, #49267
        Attached File- Added InkedScreenshot@from@2020-06-14@20-36-40_LI.jpg, #49268
        Attached File- Added InkedScreenshot@from@2020-06-14@20-45-01_LI.jpg, #49269
    2020-06-14 abdelrhman_adel Attached File- Added bug57867.patch, #49256
    2020-05-11 abdelrhman_adel Attached File- Added mypatch8.patch, #49057
    2020-05-01 abdelrhman_adel Attached File- Added 95192847_593325621580691_5460484646900858880_n.jpg, #48980
        Attached File- Added 94675326_1316051098783364_1282320047372500992_n.jpg, #48981
    2020-04-21 abdelrhman_adel Attached File- Added strrep.diff, #48894
    2020-03-28 abdelrhman_adel Attached File- Added Final2.patch, #48695
    2020-03-28 abdelrhman_adel Attached File- Added Final.patch, #48692
    2020-03-28 abdelrhman_adel Attached File- Added make@run@(bug #57867).png, #48690
    2020-03-28 abdelrhman_adel Attached File- Added patch1(bug #57867).txt, #48688
        Attached File- Added patch2(bug #57867).txt, #48689
    2020-03-24 abdelrhman_adel Attached File- Added patch(bug #57867).txt, #48656
    2020-03-23 mtmiller Dependencies- patch #9912 is dependent
    2020-03-23 abdelrhman_adel Attached File- Added strrep.patch, #48654
    2020-03-21 abdelrhman_adel Attached File- Added patch.cc, #48638
    2020-03-21 abdelrhman_adel Attached File- Added patch.txt, #48637
    2020-03-21 mtmiller Attached File#48601 Removed
    2020-03-21 mtmiller Attached File#48600 Removed

    Back to the top

    Powered by Savane 3.15-e283.
    Corresponding source code