bugGNU Octave - Bugs: bug #34713, str2double() ignores spaces and...

 
 

bug #34713: str2double() ignores spaces and multiple commas

Submitter:  Philip Nienhuis <philipnienhuis>
Submitted:  Tue 01 Nov 2011 08:54:36 AM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Philip Nienhuis Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 16 May 2015 06:51:29 PM UTC, comment #8: 

If I read this bug history correctly, the only remaining issue was whether to treat the comma as a decimal separator depending on locale?

I think we are now pretty sure that we don't want to do that, and took some steps in the most recent version to ensure even more strongly that LC_NUMERIC is set to C.

Matlab apparently ignores all commas in numeric literals, e.g.


>> str2double('1,2,3,,,,,,,,,,,,,,,4,,,,,,,,,,,,,')
ans = 1234


and Octave behaves exactly the same way now.

Closing as fixed.

Mike Miller <mtmiller>
Group Member
Wed 04 Jan 2012 04:02:29 PM UTC, comment #7: 

Everyone thinks their bug should have high priority.  How is this bug significantly different from other bug reports that it warrents high priority and important severity while so many others are just "normal"?

Does Matlab's str2double function parse commas and periods strings differently depending on locale settings?  If not, then I'm confident that we would see bug reports about Matlab compatibility if we chose to do it in Octave.

John W. Eaton <jwe>
Group administrator
Wed 21 Dec 2011 10:10:16 PM UTC, comment #6: 

I agree that this is too early to do. Potentially we could use the relevant code from somewhere else?

In fact, Octave is using GNULib somehow. As far as I understand, its sscanf together with setlocale should have the appropriate functionality. I have no idea however how this then influences other calls. If this is too complicated, I can have a look if its easy to find the relevant code in gnulib.

Kris Thielemans <krthie>
Wed 21 Dec 2011 02:25:55 PM UTC, comment #5: 

I think a change to use locales deserves more testing than we can do before the release.  A change like you propose could go on the default branch, but I think it needs to be more robust to possible errors.  For example, what happens if a locale name is not available?  Is an exception thrown?  Should we just try known locale names until one succeeds?  What if none succeed?  Do we throw an error, or just strip commas as we are doing now?

John W. Eaton <jwe>
Group administrator
Wed 21 Dec 2011 12:20:27 PM UTC, comment #4: 

It turns out that we could let istream>> do the comma handling by setting the local appropriatelly by simply removing the lines that remove the comma and setting the local there the something like en_US (see code below).

With this, all current tests succeed and we also have good handling of comma's:

octave> str2double('2,235.6 + 35,564.3i')
ans =  2.2356e+03 + 3.5564e+04i
octave> str2double('2,23,5.6')
ans = NaN

This solution is very elegant, but unfortunately has a major problem: local naming is not set by the standard (on Ubuntu 11.10 I can use "en_US.UTF-8", but Microsoft seems to use "english_US" (see http://msdn.microsoft.com/en-us/library/hzz3tw78.aspx), and it's not clear if en_US is installed on all systems in any case. I have no solution for this.

Of course, arguably str2double should use the system local anyway, but that would seem to break matlab compatibility.

$ hg diff str2double.cc
diff -r e5e74694c690 src/DLD-FUNCTIONS/str2double.cc
--- a/src/DLD-FUNCTIONS/str2double.cc Wed Dec 21 09:08:10 2011 +0000
+++ b/src/DLD-FUNCTIONS/str2double.cc Wed Dec 21 11:34:41 2011 +0000
@@ -227,13 +227,10 @@
 {
   Complex val (0.0, 0.0);
 
-  std::string str = str_arg;
+  std::istringstream is (str_arg);
+  // handle comma's
+  is.imbue( std::locale("en_US.UTF-8") );
 
-  // FIXME -- removing all commas does too much...
-  std::string::iterator se = str.end ();
-  se = std::remove (str.begin (), se, ',');
-  str.erase (se, str.end ());
-  std::istringstream is (str);
 

Kris Thielemans <krthie>
Wed 21 Dec 2011 08:34:10 AM UTC, comment #3: 

Thanks for looking into this.

FYI, ML r2007a does this:

>> str2double ('1,2,3,4')

ans =
        1234

>> str2double ('1 2 3 4')

ans =
   NaN

>> str2double ('1,234')

ans =
        1234

The first looks like a subtle ML bug to me - while commas are to be interpreted as thousands separators, they turn out to concatenate groups of any number of digits in ML r2007a.
(I'd expect only constructs with 3-digit groups to the right of the leftmost comma would be accepted, like this:

>> str2double ('1,200,300,400')

ans =
  1.2003e+009
)

Philip Nienhuis <philipnienhuis>
Group Member
Tue 20 Dec 2011 11:13:10 PM UTC, comment #2: 

I checked in the following fix for the original problem. 

http://hg.savannah.gnu.org/hgweb/octave/rev/b6eeeb67fa3f

Correctly parsing numbers like "1,234" while also disallowing "1,2,3,4" will require more work.

John W. Eaton <jwe>
Group administrator
Thu 03 Nov 2011 12:06:47 PM UTC, comment #1: 

similar weird behaviour alluded to in the bug-title, but no example was given in the original post:


octave:1> str2double('1,2,3,4')
ans =  1234

Expected result: NaN

Kris

Kris Thielemans <krthie>
Tue 01 Nov 2011 08:54:36 AM UTC, original submission:  

(see thread: https://mailman.cae.wisc.edu/pipermail/octave-maintainers/2011-October/025525.html)

octave:11> str2double ('1 2 3 4')
ans =  1234

where NaN would be expected.

Philip Nienhuis <philipnienhuis>
Group Member

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2015-05-16 mtmiller StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2012-01-04 jwe Severity4 - Important 3 - Normal
        Priority7 - High 5 - Normal
    2011-12-03 bpabbott Priority5 - Normal 7 - High
    2011-11-28 philipnienhuis Severity3 - Normal 4 - Important
    2011-11-03 bpabbott StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code