bugGNU Octave - Bugs: bug #33537, lower function returns incorect...

 
 

bug #33537: lower function returns incorect result on numerical values

Submitted by:  Arnaud Delorme <arnodelorme>
Submitted on:  Sat 11 Jun 2011 03:43:16 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: John W. Eaton <jwe>
Originator Name: Open/Closed: Closed
Release: 3.4.0Operating System: Mac OS

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)

Fri 03 Jan 2014 04:24:37 AM UTC, comment #13:

I checked in the following changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/1b6db9303933

I opted for Matlab compatibility with no warning.

This seems to fix the problem for me, so I'm closing the report. Reopen or open a new bug report if there are further problems.

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Mon 28 May 2012 08:18:14 PM UTC, comment #12:

There are some similarities with bug 36555

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

Ben Abbott <bpabbott>
Project Member
Wed 27 Jul 2011 06:57:54 PM UTC, comment #11:

I reversed my changeset here (http://hg.savannah.gnu.org/hgweb/octave/rev/ef5ebbf2a657). I'll let you decide where the appropriate fix point is in the code. A quick grep for 'umap_xisalnum' in src/*.cc shows it in the following files:

ov-base-sparse.cc:
ov-re-mat.cc:
ov-scalar.cc:

Rik <rik5>
Project Administrator
Wed 27 Jul 2011 06:17:55 PM UTC, comment #10:

Thanks for looking at this problem. I apologize for not commenting on your patch before you committed it as I maybe could have saved you some trouble.

I think it would be better to not check for types in the toupper and tolower functions. Note that there are similar problems with isascii, isdigit, etc. (all the ctype functions).

Probably the right place to fix this is in the code that deals with this case in each object. For example, ov-scalar.cc has the following code:

If you want an error message, then you could just remove the first part of the if condition that converts the argument to a character string before applying the mapper.

Or, if you want Matlab-compatible behavior, you could do that here as well by returning the value unchanged for tolower and toupper, and returning a logical false value for the other is* functions.

Similar code appears in ov-re-mat.cc. So I think this must have been an attempt to handle special cases for full double matrices in a way that was compatible with Matlab at some point in the past. I'm still OK with having error messages for incorrect types, but I think we should still simply forward in the tolower and toupper functions so that we are consistent with all the other is* mappers.

Also, forwarding for all types instead of checking types in tolower and toupper allows other types that might be added in the future to get a chance to do something useful with the toupper and tolower mappers without having to modify the DEFUNs for these functions.

I'm reopining this report so these issues won't be forgotten.

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Wed 27 Jul 2011 04:39:37 PM UTC, comment #9:

Octave now issues an error when the input is not a string or cellstr which should prevent accidentally invoking the function on numeric data. Fixed on the stable branch in this changeset (http://hg.savannah.gnu.org/hgweb/octave/rev/67bf9b30f3f9)

Rik <rik5>
Project Administrator
Sun 17 Jul 2011 04:35:36 AM UTC, comment #8:

How does the following attempt look to everyone? I made the change in mappers.cc at the DEFUN for toupper rather than at the mapping function itself. It appears to work and gives an error if the input is not a string or cell array of strings.

The old code was

Rik <rik5>
Project Administrator
Sun 12 Jun 2011 08:38:35 PM UTC, comment #7:

Some error message that char data is expected would be good.
Should be applied to upper() & toupper() & tolower() as well, then.

Just for the record, being in a similar situation I do symphatize somewhat with Arnaud (the OP), but what he wants is IMO the wrong way.
I'm also sharing loads of Matlab code with Octave and as such I'm occasionally confronted with similar cases.
But I always (try to) explain that invoking non-documented code constructs that just happen to work nicely is actually sloppy if not risky programming (on the ML side). In many a case ML is actually too forgiving - which may backfire in often unexpected cases.

Philip Nienhuis <philipnienhuis>
Project Member
Sun 12 Jun 2011 07:37:31 PM UTC, comment #6:

John,

I agree with your proposed fix. Having Octave
treat numeric data as char is not good.

Does anyone really feel that bug-for-bug compatibility
is needed here?

Michael Godfrey <godfrey>
Project Member
Sun 12 Jun 2011 07:32:40 PM UTC, comment #5:

In the case of

the argument is already converted to a character array, so this is consistent.

In the old days, Matlab did not really have character data, but just had a flag saying that a numeric array was supposed to be treated as ASCII characters. So that's probably why Octave behaves the way it does.

But if we change this, why should we change it to leave numeric data alone? Shouldn't we issue an error instead? The Matlab docs don't seem to say that lower/upper simply return their arguments if they are not character strings or cell arrays of character strings. So you are asking for bug-for-bug compatibility?

If we should have lower/upper simply return non character arguments, does that apply to everything? What happens for structures or cells containing non-string values? What about cell arrays with some character values and some other values (structs, function handles whatever)?

My fix would probably be to just throw an error if the argument is not a character string or cell array of character strings.

How does it help to allow lower/upper to be called with arguments that are not character strings or cell arrays of character strings?

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Sun 12 Jun 2011 06:01:56 PM UTC, comment #4:

Yes, I agree. Thanks for your feedback.

Arnaud Delorme <arnodelorme>
Sun 12 Jun 2011 04:35:19 PM UTC, comment #3:

I do not think that anyone would say that Matlab
is "right" in this case:

Matlab says, for more examples:

lower([ 90 100])

ans =

90 100

>>

but also:

>> lower([ 90 100 'ABC'])


ans =

zdabc

>>

This matches Octave!

Other experiments may be a good idea, but this is not
worth much time, I think. A simple solution for the
case above would be isnumeric() applied to the argument
and then do as Matlab seems to do.

Regardless of Matlab, I do not think that Octave doing:
octave:35> lower(64)
ans = @
octave:36> lower(65)
ans = a
octave:37>
is very helpful. Probably, Matlab choice would be better.

Michael Godfrey <godfrey>
Project Member
Sun 12 Jun 2011 07:12:13 AM UTC, comment #2:

lower([90])

returns

90 % matlab

returns

122 % Octave

I am not saying Matlab is right, only that this could create compatibility issues.

Arnaud Delorme <arnodelorme>
Sat 11 Jun 2011 10:31:44 PM UTC, comment #1:

The ML docs say nothing about numerical input to lower(), or for that matter, to upper()

AFAICS the ML docs do clearly state that lower() is to be applied to strings or cell arrays of strings. Same goes for upper()

So, to me it seems you're requesting some sort of "bug for bug" compatibility, or at least mimicking undocumented behaviour of ML...

Philip Nienhuis <philipnienhuis>
Project Member
Sat 11 Jun 2011 03:43:16 PM UTC, original submission:

lower() applied to numbers return the number themselves in Matlab but not in Octave. That's an important lack of compatibility. 


lower([90 100])

Arnaud Delorme <arnodelorme>

 

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

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by bpabbott (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by godfrey (Posted a comment)
  • -unavailable- added by philipnienhuis (Posted a comment)
  • -unavailable- added by arnodelorme (Submitted the item)
  •  

    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 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 03 Jan 2014 04:24:37 AM UTCjweStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Tue 02 Aug 2011 08:38:15 PM UTCjweAssigned toNone=>jwe
    Thu 28 Jul 2011 02:54:37 AM UTCrik5StatusFixed=>In Progress
    Wed 27 Jul 2011 06:17:55 PM UTCjweOpen/ClosedClosed=>Open
    Wed 27 Jul 2011 04:39:37 PM UTCrik5StatusNeed Info=>Fixed
      Open/ClosedOpen=>Closed
    Sun 12 Jun 2011 07:47:54 PM UTCjweStatusNone=>Need Info

    Back to the top


    Powered by Savane 3.1-cleanup1