bugGNU Octave - Bugs: bug #44334, isequal fails to compare size of...

 
 

bug #44334: isequal fails to compare size of objects

Submitter:  Oliver Heimlich <oheim>
Submitted:  Sat 21 Feb 2015 12:23:00 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Oliver Heimlich Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 14 Dec 2015 12:43:02 PM UTC, comment #17: 

Thank you for the fix and tests. I've pushed it to stable http://hg.savannah.gnu.org/hgweb/octave/rev/ef9b2a88fbb5

Carnë Draug <carandraug>
Group Member
Sat 12 Dec 2015 08:17:39 PM UTC, comment #16: 

Thanks Carnë, that would be great.

Julien Bect <jbect>
Fri 11 Dec 2015 02:15:27 PM UTC, comment #15: 

This all looks good to me.  Unless anyone opposes it, I will push it to stable on Monday.

Carnë Draug <carandraug>
Group Member
Fri 11 Dec 2015 05:33:50 AM UTC, comment #14: 

I had hoped that this goes into the 4.0.1 release, but the regression is still present in RC1. I didn't find it in the mercurial history.

Oliver Heimlich <oheim>
Sun 25 Oct 2015 07:53:28 AM UTC, comment #13: 

Dear Maintainers,

Please consider pushing this patch to stable before the 4.0.1 release.

Without it, several packages (interval, stk, and perhaps others ?) currently have failing unit tests.

@++
Julien

Julien Bect <jbect>
Mon 19 Oct 2015 05:57:03 PM UTC, comment #12: 

The patch looks ok to me.

I think it is ready to be pushed on stable.

(I would very much like to see this patch included in 4.0.1 !)

Julien Bect <jbect>
Mon 19 Oct 2015 04:37:13 PM UTC, comment #11: 

Your patch looks ok to me.

I am currently running a "make check" with your patch applied to the tip of the stable branch...

Julien Bect <jbect>
Mon 19 Oct 2015 04:28:05 PM UTC, comment #10: 

The new examples in R2015b help me understand the numArgumentsFromSubscript. So it has nothing to do with the size issue and is rather for {}-indexing or other custom indexing.

Can we apply the patch now?

Oliver Heimlich <oheim>
Mon 19 Oct 2015 04:17:14 PM UTC, comment #9: 

Hello Oliver,

Mathworks' documentation does provide information about overriding numel. See, e.g.,

http://fr.mathworks.com/help/matlab/matlab_oop/overloading-numel-subsref-and-subsasgn.html

for the latest release (R2015b, which by the way intrduces some novelty in this issue, with a new method called numArgumentsFromSubscript).

In short, for Matlab <= R2015a, numel be overriden consistently with subsasgn and subsref: the value returned by numel(X) corresponds to the value of nargin/nargout for subasgn/subsref when using dot-indexing.


Julien Bect <jbect>
Mon 14 Sep 2015 07:59:12 PM UTC, comment #8: 

The Mathworks' documentation on size and numel does not help understand how a class should override its public size related properties. As far as I understand, the object has a built-in size of [1 1] and is scalar by default. If both size and numel are overridden in a consistent way then everything should be well and the object is treated as if it was a different size by other functions, e. g., ismatrix, isvector, isscalar, isempty, …. If only one of size and numel is overridden, it is not quite clear how the object behaves. Also there is a method numArgumentsFromSubscript in Matlab, which is not needed/used in Octave.

Anyway, I have created a revised patch, which addresses the issues remarked by Julien. The change regarding _isequal_.m solves this bug and should be applied to the stable branch.

The supplementary test cases should now comply with Matlab semantics.

(file #34878)

Oliver Heimlich <oheim>
Wed 12 Aug 2015 09:01:45 AM UTC, comment #7: 

Hello Oliver,

Your unit test are OK for Matlab R2012a, except for

a) the tests related to rows () and columns (), which do not exist in Matlab :-), and

b) the tests related numel (). I don't think that you should expect numel to return the same result prod (size (st)). The result of numel must be defined by the class designer to be consistent with subsref and subsasgn.

Concerning your patch: if you want to speed up the process, you should provide a GNU-compliant commit message: first a one-liner and then a file-by-file description of the changes. See

https://www.gnu.org/prep/standards/html_node/Change-Logs.html

@++
Julien

Julien Bect <jbect>
Sun 09 Aug 2015 08:14:22 AM UTC, comment #6: 

Here comes a revised patch that adds unit tests.

@Julien, could you please check whether the expected results of the new unit tests are Matlab compatible?

@Maintainers, please apply the patch to the stable branch. Without the patch 16 functions of the interval package can't be verified under Octave 4.0.0.

(file #34611)

Oliver Heimlich <oheim>
Sat 18 Jul 2015 07:17:30 AM UTC, comment #5: 

@Oliver: You can write unit tests for object-oriented programming in test/classes (for old-style classes) and/or in test/classdef (for classdef-style classes).  You will find lots of test classes there, and you can add new ones if none fits your needs.

@Maintainers: Has anyone looked at Oliver's patch? It looks ok to me, and it solves a very annoying regression in 4.0.0... I would love to see it applied for 4.0.1 !

@++
Julien

Julien Bect <jbect>
Tue 16 Jun 2015 07:02:23 PM UTC, comment #4: 

And I also confirm that the patch works for me.

Julien Bect <jbect>
Tue 16 Jun 2015 05:22:42 PM UTC, comment #3: 

I confirm the bug.

Julien Bect <jbect>
Sun 14 Jun 2015 10:30:31 AM UTC, comment #2: 

Could somebody please confirm the bug and review the patch? This also affects bug #45118. Thanks.

Oliver Heimlich <oheim>
Sat 18 Apr 2015 12:03:04 PM UTC, comment #1: 

The attached patch should solve the issue.

Unfortunately I do not know how to write unit tests for objects.

(file #33709)

Oliver Heimlich <oheim>
Sat 21 Feb 2015 12:23:00 PM UTC, original submission:  

With the patch from bug #42408 (revision 18785:1f170b211be3) the following regression has been introduced: When x is an object whose class overrides the size function and x's size is not [1 1] then isequal (x, x) == false.

E.g.:

octave:1> pkg load interval
octave:2> x = infsup (magic (3));
octave:3> isequal (x, x)
ans = 0


The reason is, that the new code uses cellfun for checking sizes:

t = all (size (x,k) == cellfun ("size", varargin, k));


cellfun ("size", …) evaluates the built-in size function and not the overriden size function from the class. Thus, t will eventually be false if size (x, k) ~= 1 for any k.

Proposed fix: Use cellfun (@size, …) instead of cellfun ("size", …)

Oliver Heimlich <oheim>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #34878:  44334.patch added by oheim (4KiB - text/x-patch)
file #34611:  bug-44334.patch added by oheim (3KiB - text/x-patch)
file #33709:  bug-44334.patch added by oheim (1KiB - text/x-patch)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by jbect (Posted a comment)
  • -email is unavailable- added by jbect
  • -email is unavailable- added by oheim (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-02 siko1056 Dependencies- bugs #51308 is dependent
    2015-12-14 carandraug StatusNone Fixed
        Open/ClosedOpen Closed
    2015-09-14 oheim Attached File- Added 44334.patch, #34878
    2015-08-09 oheim Attached File- Added bug-44334.patch, #34611
    2015-05-17 jbect Carbon-Copy- Added -email is unavailable-
    2015-04-18 oheim Attached File- Added bug-44334.patch, #33709

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code