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

 
 

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

Submitter:  Arnaud Delorme <arnodelorme>
Submitted:  Sat 11 Jun 2011 03:43:16 PM UTC
   
 
Category:  None Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  jwe
Originator Name:  Open/Closed:  * Closed
Release:  * 3.4.0 Operating System:  * Mac OS
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group administrator
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>
Group 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>
Group 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:


octave_value
octave_scalar::map (unary_mapper_t umap) const
{
  switch (umap)
    {
    ...

    default:
      if (umap >= umap_xisalnum && umap <= umap_xtoupper)
        {
          octave_value str_conv = convert_to_str (true, true);
          return error_state ? octave_value () : str_conv.map (umap);
        }
      else
        return octave_base_value::map (umap);


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>
Group administrator
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>
Group 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.


DEFUNX ("toupper", Ftoupper, args, ,
    "-*- texinfo -*-\n\
@deftypefn  {Mapping Function} {} toupper (@var{s})\n\
@deftypefnx {Mapping Function} {} upper (@var{s})\n\
Return a copy of the string or cell string @var{s}, with each lowercase\n\
character replaced by the corresponding uppercase one; non-alphabetic\n\
characters are left unchanged.  For example:\n\
\n\
@example\n\
@group\n\
toupper (\"MiXeD cAsE 123\")\n\
     @result{} \"MIXED CASE 123\"\n\
@end group\n\
@end example\n\
@seealso{tolower}\n\
@end deftypefn")
{
  octave_value retval;

  if (args.length () != 1 || ! (args(0).is_string () || args(0).is_cellstr ()))
    print_usage ();
  else
    retval = args(0).xtoupper ();

  return retval;
}


The old code was


  if (args.length () == 1)
    retval = args(0).xtolower ();
  else
    print_usage ();


Rik <rik5>
Group 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>
Group 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>
Group Member
Sun 12 Jun 2011 07:32:40 PM UTC, comment #5: 

In the case of


lower([ 90 100 'ABC'])


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>
Group administrator
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>
Group 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>
Group 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 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 bpabbott (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by godfrey (Posted a comment)
  • -email is unavailable- added by philipnienhuis (Posted a comment)
  • -email is unavailable- added by arnodelorme (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-01-03 jwe StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2011-08-02 jwe Assigned toNone jwe
    2011-07-28 rik5 StatusFixed In Progress
    2011-07-27 jwe Open/ClosedClosed Open
    2011-07-27 rik5 StatusNeed Info Fixed
        Open/ClosedOpen Closed
    2011-06-12 jwe StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code