Sat 21 May 2016 08:27:12 AM UTC, comment #25:
Ok, so is it a regression (between 3.8.2 and 4.0.x) or not?
[thinking....]
-----
First element: in my initial post I wrote:
> In Octave 3.8.2, isscalar(x) returns true if numel(x) is 1.
Actually, it seems that both 3.8.2 and 4.0.x define isscalar(x) as (numel(x) == 1), but in 3.8.2 this is in an m-file, whereas in 4.0.x this in C++.
So, I agree that in term of internal implementation, isscalar seems to have been wrong, in principle, all the time.
-----
Second: the main case for a regression came, IIRC, from a comment by Oliver Heimlich (initial post of bug #44498):
> In GNU Octave 3.8.2 the overriden size had been respected by isscalar, isvector, and isequal.
I don't think I ever checked myself what was going on in 3.8.2.
-----
Thinking about Oliver's comment:
a) The case of isequal different, and has already been dealt with in bugs #44334 and #45118, so let's forget about it.
b) The case of isvector is the subject of bug #44498. In 3.8.x in was an m-file implementation calling size (which is ok) and ndims (which is not ok). So, I would say that the behaviour was ok for two-dimensional arrays and wrong for higher-dimensional arrays. In 4.0.x, issvector is implemented in C++ and used ndims(), so it never respects the oveloaded size. Summary for the case of issvector: for isvector, we have a Matlab-compatibility regression in the case of two-dimensional arrays.
c) The case of isscalar still puzzles me. Given my remark above, I now think It has been wrong all the time, being based on numel. The only explanation that I can think of is that Oliver had numel overloaded in his test class, equal to prod (size (x)).
-----
About some other functions:
d) rows, columns: The behaviour hasn't changed. Overloaded size has never been respected. Note that these functions do not exist in Matlab. Therefore, as far as they are concerned, Matlab-compatibility is not an issue. Only consistency with the other functions is.
e) isrow, iscolumn: Same case as isvector. 3.8.x had an m-file implementation based on size and ndims => Matlab-compatiblity regression for two-dimensional arrays.
f) issquare : 3.8.x also had an m-file implementation based on size and ndims, which thus correctly respected overloaded size functions in the case of two-dimensional arrays. Now it doesn't respect it at all. This is a change of behaviour, but not a Matlab-compatinility issue since the function does not exist in Matlab.
g) ismatrix: Very different case. Was not at all Matlab-compatible in 3.8.x (this has been discussed elsewhere). The 4.0.x version is better in terms of Matlab compatiblity, but ignores the overloaded size.
-----
Summary: I believe that
- we have several regressions between 3.8.2 and 4.0.x wrt supporting overloaded size: isvector, isrow, iscolumn.
- fixing these functions but not the others would be a bad idea for two reasons: 1) Even they have not regressed, the other functions are faulty in terms of Matlab-compatibility, and 2) all these size-related functions should behave similarly.
-----
Sorry for the very long post.
Now, the decisions to be made is: do we fix this on stable, on default, or do we fix the regressions only on stable and the rest of it on default ?
I would vote for fixing everything on stable...
|
Fri 20 May 2016 10:47:38 PM UTC, comment #24:
I can see that this bug is a Matlab compatibility problem that should be fixed, but I'm having a hard time understanding what the regression is.
I've made a simple class example that overrides the size method and I don't see any change at all w.r.t isscalar (incorrectly depends on numel in all versions), so that is not a regression.
I do see differences in isvector and ismatrix, but I don't think either of them were absolutely correct in 3.8 and broken in 4.0, so I wouldn't call that a regression either. They may be broken in both 3.8 and 4.0 in different ways, but that's not really a regression, is it?
I'm not trying to be difficult, just trying to understand to help find the best way to resolve this. Can you show me what I'm missing?
Granted this is a somewhat constructed example showing that isvector was buggy in 3.8 as well. If I change the size to [2,2,3], then isvector correctly returns false in 3.8 and true in 4.0 and newer.
|
Fri 20 May 2016 04:33:02 PM UTC, comment #23:
Following up on comment #22:
Here is the definition of octave_base_value::rows() in 3.8.2:
It is not overridden in octave_class.
So, if I understand correctly, the (undocumented) behaviour of octave_base_value::rows() for objects has always been to return the size of the underling struct container, even when size is overloaded to return something different.
Therefore, I believe that this should not be changed as part of this regression fix.
|
Fri 20 May 2016 04:15:31 PM UTC, comment #22:
@mike, bis (comment #20)
About making value.rows() in C++ return the same thing as rows(x) in the interpreter: I agree that it make sense, but is it a regression, or was it alwas like that?
If the old (3.8.x) behaviour was to return the same value, I would say that this is part of the regression, and therefore must also be fixed in stable. I can prepare a modified patch for that.
If the old behaviour was to return different values, I would say that we should only fix the regression on stable (using the proposed patch) and then discuss this design issue somewhere else (same as comment #21).
|
Fri 20 May 2016 04:00:30 PM UTC, comment #21:
@Mike (comment #20)
About adding the "const" attribute to size: assuming it is possible, I don't believe such a change would be made on stable, would it?
My patch applies to stable, since it fixes a regression in the 4.0.x series, and I hoped it could be applied before 4.0.3 gets released.
Perhaps could we keep the two issues separate:
- first fix the regression on stable and close this bug report,
- second open a separate bug report (or ticket on the patch tracker) to discuss the const-ness of size and perhaps change it?
|
Thu 19 May 2016 07:13:40 PM UTC, comment #20:
I think I understand what's going on here. At least you seem to have done thorough investigation into the differences between ndims, numel, and size.
I also wonder why size is not const, I suspect it's only because octave_class::size calls octave_class::in_class_method, which calls octave_class::find_parent_class which is non-const. We can easily check for other overrides of octave_base_value::size (at least in Forge repositories) to see if there are any non-const usages out there.
I also wonder if we shouldn't fix octave_class::columns and octave_class::rows along with this change, so that at least stays consistent. I would find it confusing if rows(x) returns one thing at the interpreter level, but value.rows() returns something completely different in C++.
|
Mon 16 May 2016 08:32:16 AM UTC, comment #19:
Here is a slightly improved version of my patch.
I believe it is now ready to be applied on stable (since it fixes a regression).
(file #37176)
|
Wed 17 Feb 2016 07:44:53 AM UTC, comment #18:
About Fndims, see http://lists.gnu.org/archive/html/octave-maintainers/2016-02/msg00075.html
Unless something new comes up, I won't try to changes Fndims's behaviour in this patch.
|
Fri 12 Feb 2016 01:23:45 PM UTC, comment #17:
See the attached patch for work in progress.
It now deals with more functions than we originally thought (issquare, isrow, iscolumn, rows, columns...).
I am still not sure of the right course of action concerning Fndims (see below: Do we assume that it is a bug in Matlab and fix it ?).
Some more work is also required on classes.tst (add new unit tests, check for Matlab compatibility).
(file #36323)
|
Fri 12 Feb 2016 10:22:45 AM UTC, comment #16:
By the way, I understand now why tests such as (sz(0) >= 0) are useful. The reason is that octave_class doesn't fully check that the result of size, when it is overloaded, is a valid size vector. So, sz(0) could be negative, Nan or infinite...
|
Fri 12 Feb 2016 10:20:11 AM UTC, comment #15:
Well, I leave it to you to discuss it on the mailing list if you like.
As far as this patch is concerned, we have to make a copy to avoid the problem (size() might become const one day, but certainly not in a 4.0.x release).
|
Fri 12 Feb 2016 09:15:24 AM UTC, comment #14:
I also found it odd that size() isn't const.
I didn't raise it because it is difficult to change. There is probably code out there that overloads the non-const size(). If we make size() const, that code will fail in complicated ways. That is even true if we have both a const and non-const size(), and we use the const one.
However, that is just my opinion. I haven't seen it discussed on the mailing list, but it would be great to hear what others think.
|
Fri 12 Feb 2016 08:28:16 AM UTC, comment #13:
Hello Lachlan,
I see that we have to copy args(0) because size() isn't const.
I find it weird that size() isn't const but numel, ndims, rows, columns... all are!
Is there a rationale for this? Has this been discussed on the mailing list?
@++
Julien
|
Thu 11 Feb 2016 12:49:48 PM UTC, comment #12:
I have a doubt about ndims()... It would seem natural to me that ndims() should also respect the user-defined size.
But in Matlab (R2012a) this is confusing:
- The documentation says that ndims() should return length(size(x)).
- But ndims does not respect that is the size function is overloaded...
I have to check whether this has been fixed in more recent Matlab versions.
|
Thu 11 Feb 2016 12:40:33 PM UTC, comment #11:
I will prepare a new patch, so until I'm done, yes, you can leave it to me.
|
Thu 11 Feb 2016 12:35:12 PM UTC, comment #10:
Thanks. I was just about to say that the sz(0) >= 0 was introduced in changeset 00e31f316a3a, with the change
I forgot to update the unit tests. That is a habit I should get into with all patches, I suppose. Is there anything else I can help with on this, or should I leave it to you?
|
Thu 11 Feb 2016 12:30:17 PM UTC, comment #9:
Ooops, there are so many typos in my previous message that if find it hard to read... I meant:
For matlab compatibility, ismatrix must return true for sizes such as 0x1 or 1x0. The rule is that ismatrix returns true if the size is [m n] for non-negative integers m and n.
I have just tested without the sz>=0 tests, and all the unit tests still pass. I will prepare a modified patch later.
In the meantime, I am currently which xtest statements in test/classes/classes.tst can now be converted to test.
I have also found out that columns() and rows() suffer from exactly the same problem. I will extend the patch to deal with them as well.
|
Thu 11 Feb 2016 12:17:47 PM UTC, comment #8:
For matlab compatibility, ismatrix must return true if for sizes such 0x1 or 1x0. The rule is that ismatrix returns true is the size is [m n] for non-negative integers m and n.
|
Thu 11 Feb 2016 11:02:24 AM UTC, comment #7:
Good catch regarding sz(.) >= 0. I just copied what was there...
My guess is that they should be sz(.) > 0, since something isn't really a matrix if it is empty. Or perhaps they used to be >0, and someone "fixed" them to be >=0 to explicitly allow empty matrices. I'll take a look for that.
|
Wed 10 Feb 2016 04:09:50 PM UTC, comment #6:
I have rebased the patch to stable (since it fixes a regression, I believe it should be included in 4.0.1) and added a similar fix for issquare.
I have tested all four functions (isscalar, isvector, ismatrix, issquare) and everything looks fine to me.
The tests in ismatrix (sz(0) >= 0 && sz(1) >= 0) should be removed if there is no reason for keeping them. Otherwise, I think that a comment should be added to explain why they are necessary.
(file #36300)
|
Wed 10 Feb 2016 01:06:14 PM UTC, comment #5:
What is the point of testing for sz(0) >= 0 && sz(1) >= 0 in ismatrix ?
|
Wed 10 Feb 2016 12:47:22 PM UTC, comment #4:
I suggest retagging this bug as regression (same as bug #44498) since the problem didn't exist in Octave 3.8.2.
|
Sat 30 Jan 2016 02:41:41 AM UTC, comment #3:
The attached patch fixes this.
It also fixes bug #44498 (isvector), and "ismatrix" which seems not to have been reported.
Could someone please test it, and (someone else?) apply it? Thanks.
(file #36205)
|
Sun 04 Jan 2015 03:16:52 PM UTC, comment #2:
I have updated the suite of unit test for user-defined classes to include additional tests that fail on this isscalar problem.
Please consider pulling from my octave repo:
https://bitbucket.org/jbect/octave
the four most recent changesets (b1de63b, 778c39d,
1ee4035 and f0a5dd0).
|
Sat 03 Jan 2015 06:47:30 PM UTC, comment #1:
Please also see bug #42422 for a similar problem with ismatrix.
|
Sat 03 Jan 2015 06:24:46 PM UTC, original submission:
In Octave 3.8.2, isscalar(x) returns true if numel(x) is 1.
In Matlab, isscalar(x) returns true if size(x) is [1 1].
http://www.mathworks.com/help/matlab/ref/isscalar.html
These two conditions are not always equivalent.
For instance, in the case of a user-defined class that overloads size, it is possible to have size (x) equal to [6 4] but numel (x) equal to 1. (And overloading numel too is not always an option, since the result of numel matters for subsref.)
isscalar has been rewritten in C++ on the default branch, but the behaviour is still the same.
|