bugGNU Octave - Bugs: bug #52479, textscan ignores leading spaces...

 
 

bug #52479: textscan ignores leading spaces when creating cell from string/file

Submitted by:  Motius <motius>
Submitted on:  Wed 22 Nov 2017 01:11:33 PM UTC  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Matlab Compatibility
Status: FixedAssigned to: None
Originator Name: MotiusOpen/Closed: Closed
Release: devOperating System: GNU/Linux

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 Jump to the original submission)

Wed 29 Nov 2017 05:08:34 AM UTC, comment #13:

I opened a new bug report for the observation in comment 12:

https://savannah.gnu.org/bugs/index.php?52550

because this seems different enough from the original report and is not related to white-space. Consider that similar behavior occurs when the delimiter is a comma:

Let's keep this report concise and not conflate it with the differences between string-specified and cell-specified delimiters. I'll address various things, including compatability at the new bug report. We can close this one.

Dan Sebald <sebald>
Sat 25 Nov 2017 01:34:58 AM UTC, comment #12:

I checked in the change on stable here (http://hg.savannah.gnu.org/hgweb/octave/rev/ddc91a2ee0e0). That still leaves a problem when the delimiter is specified in a cell array, rather than as a single character. When specified as a single character, the code works.

But when the same character is presented as part of a cell array it is one entry too short.

Someone want to take a look at that? It is no longer a regression, so I'm changing the report status to "Matlab Compatibility".

Rik <rik5>
Project Administrator
Fri 24 Nov 2017 07:50:01 PM UTC, comment #11:

Updated patch looks nice. I imagined there must be a C++ routine that removes a letter from a string, but I could find no such function. I wasn't aware of the iterator-based <algorithm>.

Dan Sebald <sebald>
Fri 24 Nov 2017 10:37:38 AM UTC, comment #10:

yields

with Matlab R2017a (windows)

Motius <motius>
Fri 24 Nov 2017 10:05:44 AM UTC, comment #9:

Sorry, I forget that Matlab has introduced string arrays that use double quotes as a creation symbol. I meant to create a string and process that.

Rik <rik5>
Project Administrator
Fri 24 Nov 2017 09:28:02 AM UTC, comment #8:

Hello,

results of:

is:

with Matlab R2017a (Windows®).

I don't think that the code I'm working with uses double quotes (cf. 'a' variable)...

Motius <motius>
Thu 23 Nov 2017 11:18:59 PM UTC, comment #7:

Can the original reporter try the following in Matlab?

In Octave, this is returning a 4x1 cell array. Whereas with just '\t' a 5x1 cell array is returned.

Also, I updated the patch from comment #2 and added BIST tests for this bug report in file-io.cc.

(file #42464)

Rik <rik5>
Project Administrator
Thu 23 Nov 2017 10:17:54 PM UTC, comment #6:

Aside from the larger architectural issues around strread and textscan, is the patch in comment #2 good enough to resolve this bug? I can review for style and commit if it is.

As for tests, there are already a bunch of tests for textscan in the file file-io.cc. Try

to run them.

Rik <rik5>
Project Administrator
Thu 23 Nov 2017 08:53:43 PM UTC, comment #5:

Indeed the plan has always been to morph textscan() into a backend for strread. In fact I started that soon after Lachlan's binary textscan was accepted into core Octave but I didn't pursue that due to lack of time, and didn't pick up again.

At the time I tried to make strread.m as Matlab-compatible as I could; I think unconditionally removing spaces isn't the right thing to do.

Note that current strread.m can do some things that Matlab's strread nor Octave&Matlab's textscan can do, i.e., processing cuddling literals; see e.g., this strread.m test:

and I often use that capability so I'd like to keep strread.m somehow.

As to tests, I'm sure there have been tests for binary textscan() but I didn't see them anymore in the fntests.log

Philip Nienhuis <philipnienhuis>
Project Member
Wed 22 Nov 2017 10:54:29 PM UTC, comment #4:

comment #3: OK, having read the textscan help, the takeaway is that textscan is a superset of strread() except for possibly different treatment of whitespace characters? Hence, it is difficult to make one or the other a main source routine and re-using it for the other routine? strread is a script, so is it slower in some significant way? It might be better if strread() utilized textscan() similar to what you described with "whitespace set to "\t\b"" (instead of going the other way with " \t\b".

Here's the white-space treatment from strread.m:

Unlike the above, the patch I submitted is always removing the delimiters regardless of whether the format is %s or numeric.

As an aside, it would be nice if built-in textscan() had tests just like scripts have tests so that one could tweak some code and immediately get feedback on what the ramifications are.

Dan Sebald <sebald>
Wed 22 Nov 2017 10:31:16 PM UTC, comment #3:

Swapping delimiters from whitespace was also done in the old .m-version of textscan (actually in its workhorse strread.m).

FWIW:
What also might need some attention (ISTR I mentioned this earlier as well) is that as soon as a "%s" (string) format conversion specifier occurs, whitespace behaves a bit differently.
That was my primary motive to add the '"whitespace", ""' option/value pair in comment #1.
This is where the default whitespace & delimiter characters collection comes in.

All this has been implemented in strread.m, see L.409-428. Note that textscan.m called strread.m with default (= if not overridden) whitespace set to (IIRC) " \t\b" (note space). These days I'm unsure if space really belongs to Matlab's textscan default whitespace collection.

Philip Nienhuis <philipnienhuis>
Project Member
Wed 22 Nov 2017 08:12:00 PM UTC, comment #2:

It looks like the scanning routines all do skipping of white space in some form:

However, at the instantiation, the default white space includes the \t character:

To confirm this hypothesis, note that using a comma as a delimiter works:

What needs to be done is remove any delimiters from the whitespace string. That is, if the user selects

as you've done, then that character should be subtracted from the string.

I'm attaching a patch that does just that. It seems to be in a reasonable location for modifying the whitespace list, although I do see a lot of "isspace()" used throughout the textscan code; so whether the patch catches everything, I'm not sure. Perhaps JWE and Rik can review for a more efficient use of strings or general fix.

(file #42460)

Dan Sebald <sebald>
Wed 22 Nov 2017 06:52:40 PM UTC, comment #1:

Confirmed on dev octave.

The following may be a clue:

...so this may have to do with the default whitespace / delimiter collection.

ISTR to have mentioned this (= default whitespace/delimiter) earlier on in another bug report about textscan in Octave >= 4.2.x

Philip Nienhuis <philipnienhuis>
Project Member
Wed 22 Nov 2017 01:11:33 PM UTC, original submission:

SYNOPSIS

The following snippet illustrates a regressions in the textscan GNU octave function.

EXAMPLE

RESULTS

Gives the following result under GNU octave v4.0.3:

and the following result under GNU octave v4.2.1 (1a8b7b3ae5f2):

MATLAB

The second one differs from MatLab behaviors.

OTHER

History

I initially reported this bug in the comments of bug 50485:
https://savannah.gnu.org/bugs/?50485#comment3

Tests

This was tested on debian GNU/Linux 9.1:
- Octave 4.0.3 in the repositories
- Octave 4.2.1 compiled from source (hg id: 1a8b7b3ae5f2)

Motius <motius>

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #42464:  52479.cset1 added by rik5 (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by sebald (Updated the item)
  • -unavailable- added by motius (Submitted the item)
  • -unavailable- added by motius
  •  

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

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 29 Nov 2017 05:35:21 AM UTCrik5StatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Sat 25 Nov 2017 01:34:58 AM UTCrik5Item GroupRegression=>Matlab Compatibility
      StatusNeed Info=>In Progress
      Release4.2.1=>dev
    Fri 24 Nov 2017 10:05:44 AM UTCrik5StatusNone=>Need Info
    Thu 23 Nov 2017 11:18:59 PM UTCrik5Attached File-=>Added 52479.cset1, #42464
    Wed 22 Nov 2017 08:12:00 PM UTCsebaldAttached File-=>Added octave-textscan_whitespace_delimiter-djs2017nov22.patch, #42460
    Wed 22 Nov 2017 01:11:33 PM UTCmotiusCarbon-Copy-=>Added -unavailable-

    Back to the top


    Powered by Savane 3.1-cleanup1