Wed 14 Jun 2017 07:03:02 PM UTC, comment #16:
Rik, thanks for checking.
Later on I'll come up with some doc fixes as regards char array processing by ismember - we cannot stay behind Matlab there can we.
|
Tue 13 Jun 2017 04:51:48 PM UTC, comment #15:
I checked the examples in comment #14 in Octave and they produce the same results as Matlab. I think we're okay, and no need to add a 'legacy' flag.
|
Tue 13 Jun 2017 04:46:15 PM UTC, comment #14:
W/o reopening (yet), hereś an excerpt from an email thread I had with TMW tech support:
<QUOTE>
It is not specifically documented that for "ismember", character matrices the trailing spaces are ignored. Also, it is not documented that the behavior for character matrices is different than for character vectors. It should be though and this is one of the things I will be discussing with our documentation team.
</QUOTE>
So we get this:
TMW (IMO correctly) mentioned that char arrays are actually a legacy type; one motive for truncating trailing spaces in char arrays by ismmber() is because there's no way to distinguish genuine input spaces from spaces originating from padding the char array to get rectangular.
I cannot check Octave's behavior now (busy bisecting s/th) but I think we'd better follow Matlab in truncating trailing spaces in char arrays, no matter how inconsistent.
Or, we could add a "legacy" flag (r2016b still has it) but as you mentioned that may be a bit far out for an obscure "bug" like this
If you want I could have a shot at it, though.
|
Sat 10 Jun 2017 07:45:54 AM UTC, comment #13:
Works fine on Linux and Windows.
Closing report
|
Fri 09 Jun 2017 06:32:09 PM UTC, comment #12:
I agree, I dobut you'll get much satisfaction out of TMW's tech support. In the meantime, I committed the simplest fix which still seems to resolve this issue. See http://hg.savannah.gnu.org/hgweb/octave/rev/21b906769c0d. I also added two BIST tests from this bug report. Marking as "Ready for Test".
|
Fri 09 Jun 2017 05:22:42 PM UTC, comment #11:
In the mean time I entered a bug report on the TMW site as the change in treatment of char arrays by ismmber is nowhere documented.
The usual answer I get from TMW when filing this sort of bugs is that it is rather a "doumentation issue" (I interpret that they're too ignorant to fix bugs and just changing the docs is the least amount of work).
So I agree that we should copy the behavior of the latest Matlab release - chances that this is gonna be fixed can be considered slim.
Another thing (that I added in that bug report) is that the "legacy" flag already existed in r2012a, and there it served to relax type checking / acceptance of ismember input arguments. So the purpose and results of that 'legacy' flag seem to have changed from release to release (.... no comment)
|
Fri 09 Jun 2017 04:33:47 PM UTC, comment #10:
It seems like the behavior is very complicated. In the interest of minimizing the coding burden I would not support the 'legacy' flag and only support the most recent behavior, whatever it is.
As for cellstr and the trimming of spaces at the end, that is Matlab compatible behavior which needs to be kept. But notice that the C++ code for cellstr uses an argument to the Cell constructor when it creates the Cell.
The constructor for Cell has several variations. The one in question is
One potential coding path would be to have a function, maybe one for internal use with '__' prefix and suffix, which calls Cell (sv, false) so that spaces are not trimmed.
Another option would be examine the code for lookup which is what ismember uses internally. The relevant code is
Maybe cell_value rather than cellstr_value would be more appropriate for the initialization of str_table?
Or maybe the problem is with y. In the m-file for either ismember or lookup single row char arrays could be converted to cells. This seems to work with the examples given.
A quick diff for that change is
'test ismember' still passes which is a good thing, and the failing case also passes.
|
Fri 09 Jun 2017 01:58:59 PM UTC, comment #9:
OK I checked and it turns out that Matlab r2012a and r2016b behave differently as regards char arrays. For simple character strings (1D vectors) the behavior is as I wrote in comment #8:
R2016b
======
However with r2016b:
R2012a
======
The 'legacy' option seems to have been made especially for Tables and other new stuff like datetime arrays, duration arrays and timetables; although it existed already in r2012a where it apparently was meant to relax stricter rules regarding input types.
I wonder if treatment of 2D char arrays in recent matlab releases might have been an unintended "regression".
I didn't find info in Matlab's release notes, so I might enter a bug report for it in the Mathworks site.
|
Fri 09 Jun 2017 09:28:14 AM UTC, comment #8:
IIRC correctly (I already tried that this week) Matlab returns a single 1. In the online ML docs there's a similar example.
Later today I'll verify.
I attach a preliminary fix using a for loop (yes, ugly, but I couldn't find a vectorized way to convert a char array into a cellstr array w/o truncation trailing whitespace)
I have another case that needs attention and that I was busy trying to fix as well:
Matlab (r2016b):
Octave, with the attached patch:
(file #40891)
|
Thu 08 Jun 2017 09:08:24 PM UTC, comment #7:
Could someone check what Matlab returns for
I'm interested in knowing whether it returns a single value, or whether it treats the 'b' and ' ' separately so it returns [1 1].
|
Wed 07 Jun 2017 08:02:56 PM UTC, comment #6:
@Guillaume, comment #5:
In the inline Matlab docs (r2017a) for ismember there's a stanza that clearly indicates that Matlab's ismember honors trailing spaces.
If your first example came from Matlab, I think you've hit a Matlab bug (what ML version did you use then?)
Your suggestion for a fix won't work as it invokes cellstr() which is at the heart of the problem - that function strips the trailing spaces.
We need a way for turning a char array into a cellstring array while preserving trailing spaces.
Status -> in progress
|
Wed 07 Jun 2017 11:01:24 AM UTC, comment #5:
When the input is a char array, it seems that it is transformed to a cell array in Matlab (with spaces stripped):
so a fix could be:
|
Tue 06 Jun 2017 07:01:46 PM UTC, comment #4:
Using cellstr and then adding back stripped spaces doesn't seem good to me.
I don't think you can simply substitute {} for cellstr because of the difference between
|
Tue 06 Jun 2017 12:12:40 PM UTC, comment #3:
The attached cset helps, but I'm unsure if it is the right fix.
Its effect is that it simply turns character arguments (#1 and/or #2) of ismember.m into a plain 1x1 cell array. AFAICS it also works with char arrays but I haven't tested it extensively.
The underlying problem is that cellstr() drops trailing spaces (BTW Matlab does the same), so we must somehow avoid cellstr() or implement a way to add back trailing spaces after a call to cellstr() (which will probably boil down to a kludge).
(file #40863)
|
Tue 06 Jun 2017 11:20:57 AM UTC, comment #2:
BTW, trying with a simple space " " rather than "a " is sufficient
|
Tue 06 Jun 2017 11:19:31 AM UTC, comment #1:
Compared with Matlab r2016b (example works fine there) and confirmed with dev version as well.
OS -> Any
Release -> dev
|
Tue 06 Jun 2017 05:58:54 AM UTC, original submission:
octave:36> version
ans = 4.0.3 ## installed via backports.
octave:37> a = "a "; ismember(a,{a})
ans = 0
.. on Debian 8 jessie stable.
|