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.
|
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
|
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:
|
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.
|
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)
|
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
|
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.
|
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?
|
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?
|
Sun 12 Jun 2011 06:01:56 PM UTC, comment #4:
Yes, I agree. Thanks for your feedback.
|
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.
|
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.
|
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...
|
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])
|