bugGNU Octave - Bugs: bug #42735, subclass inheritence: symbol ops...

 
 

bug #42735: subclass inheritence: symbol ops (e.g., plus, mtimes) do not respect superiorto

Submitter:  Colin Macdonald <cbm>
Submitted:  Fri 11 Jul 2014 11:08:52 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  siko1056
Originator Name:  Open/Closed:  * Closed
Release:  * 4.4.1 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 05 Oct 2018 01:31:38 PM UTC, comment #21: 

Don't worry. And thanks for the quick fix.
The tests pass for me now on Windows, too.

Closing as fixed again.

Markus Mützel <mmuetzel>
Group administrator
Thu 04 Oct 2018 10:17:16 PM UTC, comment #20: 

You are right, that was my fault.  I did not test the source code distribution, just the repository :(

I hope this fixes the situation and sorry for the inconvenience: https://hg.savannah.gnu.org/hgweb/octave/rev/72f9d4f971e5

Kai Torben Ohlhus <siko1056>
Group Member
Thu 04 Oct 2018 09:18:59 PM UTC, comment #19: 

That's right: Those files aren't present in the installation in Windows.
They probably need to be added to test/classes/module.mk.

Do you want to do that?

Re-opening until this is done.

Markus Mützel <mmuetzel>
Group administrator
Thu 04 Oct 2018 08:37:29 PM UTC, comment #18: 

@Markus: Can you please double check, that you have a "clean" building and source directory without stale files or cached values?

The sentence "plus method not defined for CPrecedenceTester1" makes me suspicious, that files like "test/classes/@CPrecedenceTester1/plus.m" where not applied in your building environment.  Those have been part of the changeset of comment #14.

Kai Torben Ohlhus <siko1056>
Group Member
Thu 04 Oct 2018 08:05:18 PM UTC, comment #17: 

This doesn't seem to work on Windows with a version from the default branch (hg id 3f2282954600).
The failing tests are:

>>>>> processing D:\Repositories\Octave\test\octave-2018-10-04-20-46-w64_clean\octave-2018-10-04-20-46-w64\mingw64\share\octave\5.0.0\etc\tests\fixed\classes\classes.tst
***** assert <*42735> (isequal (class (A + B), 'Snork'))
!!!!! regression: https://octave.org/testfailure/?42735
invalid conversion from class to double
shared variables   scalar structure containing the fields:

    A =

      <class Snork>

    B =

      <class CPrecedenceTester1>

***** assert <*42735> (isequal (B + A, 'CPrecedenceTester1'))  % idem
!!!!! regression: https://octave.org/testfailure/?42735
plus method not defined for CPrecedenceTester1 class
shared variables   scalar structure containing the fields:

    A =

      <class Snork>

    B =

      <class CPrecedenceTester1>

***** assert <*42735> (isequal (A + B, 'CPrecedenceTester2'))
!!!!! regression: https://octave.org/testfailure/?42735
plus method not defined for CPrecedenceTester2 class
shared variables   scalar structure containing the fields:

    A =

      <class Snork>

    B =

      <class CPrecedenceTester2>

***** assert <*42735> (isequal (B + A, 'CPrecedenceTester2'))
!!!!! regression: https://octave.org/testfailure/?42735
plus method not defined for CPrecedenceTester2 class
shared variables   scalar structure containing the fields:

    A =

      <class Snork>

    B =

      <class CPrecedenceTester2>

***** assert <*42735> (isequal (class (A + B), 'Snork'))
!!!!! regression: https://octave.org/testfailure/?42735
invalid conversion from class to double
shared variables   scalar structure containing the fields:

    A =

      <class Snork>

    B =

      <class CPrecedenceTester3>

***** assert <*42735> (isequal (class (B + A), 'Snork'))
!!!!! regression: https://octave.org/testfailure/?42735
invalid conversion from class to double
shared variables   scalar structure containing the fields:

    A =

      <class Snork>

    B =

      <class CPrecedenceTester3>


Is the code for the precedence rules platform dependent? Or is this a different bug?

Markus Mützel <mmuetzel>
Group administrator
Tue 02 Oct 2018 12:19:16 PM UTC, comment #16: 

On this tracker, I did not find a corresponding report for the precedence issue with classdef.

@cbm: Can you open another report with a short example demonstrating what works, what not, and what works in Matlab, including a reference to this item?
Otherwise, my lazy approach was to wait until a user needing this feature complains, what is likely to happen ;-)

Kai Torben Ohlhus <siko1056>
Group Member
Tue 02 Oct 2018 06:56:51 AM UTC, comment #15: 

Thanks!  This does indeed fix my downstream issue in Symbolic.

I guess we need a new issue open for classdef?

Colin Macdonald <cbm>
Fri 28 Sep 2018 10:41:45 AM UTC, comment #14: 

I went ahead and pushed the patch:

https://hg.savannah.gnu.org/hgweb/octave/rev/868830474750

Closing this report, as the original issue is resolved.

Kai Torben Ohlhus <siko1056>
Group Member
Wed 26 Sep 2018 12:45:28 PM UTC, comment #13: 

Using old style classes, I also came across this bug and found hopefully an elegant and satisfying solution to this problem in the attached patch (file #45100), following the advice from Mike of comment #12 and comment #3.

This patch solely addresses the old style classes and adds some regression tests.  If you agree, I would like to push it to the stable branch to become part of Octave 4.4.2.

Kai Torben Ohlhus <siko1056>
Group Member
Sun 23 Apr 2017 10:47:06 PM UTC, comment #12: 

In Colin's original example of two old-style classes that are not related by inheritance, the only thing that needs to be implemented is calling the correct operator method. I already pointed to the relevant lines of code in comment #3, this is still valid and I believe the correct place to solve this.

I haven't looked at all into how much work is needed on the classdef side, but the parser work seems to have been done a long time ago, the properties are already parsed and stored in the metaclass, that information just needs to be used elsewhere, probably passed on to the symbol table the way it is done currently in the inferiorto and superiorto functions.

Mike Miller <mtmiller>
Group Member
Sun 23 Apr 2017 10:33:32 PM UTC, comment #11: 

The parser is already handling and storing all of the information from calls to inferiorto, superiorto, and the classdef InferiorClasses property. These bits just need to be used when determining how to dispatch function and method calls.

Look at the metaclass to verify that all of the class properties have been parsed correctly


>> d = D;
>> metaclass(d).Name
ans = D
>> metaclass(d).InferiorClasses{1}.Name
ans = A


I would first test all of the examples posted on this bug report from 2014 on the current default branch, because several changes have been made to how method dispatch and overloading is implemented. Then decide what is working and what still needs to be completed, both for old classes and classdef classes.

Mike Miller <mtmiller>
Group Member
Fri 24 Mar 2017 07:21:04 PM UTC, comment #10: 

Hi, can anyone familiar with the parser please let me know where to begin to solve this. I still can't find where to look. if a class is defined as "classdef (InferiorClasses = {?A}) D < A" how do I make sure to read this info in the definition of class D? For normal classes, 'superiorto' works where the class is provided as argument. But for classdef we need this syntax and I have no clue how to parse this.

Abhinav Tripathi <genuinelucifer>
Sat 04 Feb 2017 07:36:50 AM UTC, comment #9: 

I don't know the answer to your question.  But does it help that old-style classes also exhibit this bug?  Does Octave do anything (parsing for example) with "superiorto"?

Colin Macdonald <cbm>
Sat 04 Feb 2017 07:12:26 AM UTC, comment #8: 

I was trying to work on this issue but am totally lost on how to implement the 'InferiorClasses' thing. How to process something like 'classdef (InferiorClasses = {?A}) D < A' to get all the inferior classes in c++ code? Is this information currently stored anywhere? I couldn't find anything that would help me to add a new parsing logic to parse such phrases.

Abhinav Tripathi <genuinelucifer>
Thu 19 Jan 2017 05:17:32 AM UTC, comment #7: 

Note, that Mike already said all this below, but now I understand it ;-)  Maybe my A,B,C,D classes can be worked into a test case.

Colin Macdonald <cbm>
Thu 19 Jan 2017 05:15:44 AM UTC, comment #6: 

Aha, yes "InteriorClasses" is key.  Now D is defined as "classdef (InferiorClasses = {?A}) D < A"

Matlab

>> a = A(10); b = B(20); c = C(30); d = D(40);
>> a + d         %% note
called D::plus

ans =

       40010

>> d + a
called D::plus

ans =

       10040


Octave:

octave:1> a = A(10); b = B(20); c = C(30); d = D(40);
octave:2> a + d     %% oh no
called A::plus
ans =  60
octave:3> d + a
called D::plus
ans =  10040
octave:4>



(file #39509)

Colin Macdonald <cbm>
Thu 19 Jan 2017 05:10:38 AM UTC, comment #5: 

Ok, I've done some tests with three classdef classes: A and B are unrelated.  C is a subclass of A.  Attached below.

Here is what Matlab 2015b does:

>> a = A(10); b = B(20); c = C(30);
>> a + b
called A::plus

ans =

    40

>> b + a
called B::plus

ans =

    50

>> c + a
called C::plus

ans =

        1030

>> a + c        %% note
called A::plus

ans =

    50

>>


Note "a + c" calls the A@plus!  That was unexpected: I suppose that is what "InferiorClasses" is all about.  Will check that next..., stay tuned.

So far, the results from Octave are the same:

octave:1> a = A(10); b = B(20); c = C(30);
octave:2> a + b
called A::plus
ans =  40
octave:3> b + a
called B::plus
ans =  50
octave:4> c + a
called C::plus
ans =  1030
octave:5> a + c
called A::plus
ans =  50
octave:6>



(file #39506, file #39507, file #39508)

Colin Macdonald <cbm>
Tue 01 Nov 2016 05:42:47 AM UTC, comment #4: 

This page (https://www.mathworks.com/help/matlab/matlab_oop/object-precedence-in-expressions-using-operators.html) suggests that if two user-defined classes are completely unrelated, the left-hand object's method is used (which is what Octave implements today).

So the logic that is lacking here is relationships between class inheritance (both old and new style OO), old-style superiorto/inferiorto relationships, and new-style InferiorClasses relationships.

Mike Miller <mtmiller>
Group Member
Tue 01 Nov 2016 05:29:17 AM UTC, comment #3: 

The cause of this behavior is right here


    std::string dispatch_type
      = a1.is_object () ? a1.class_name () : a2.class_name ();


http://hg.savannah.gnu.org/hgweb/octave/file/b5407b1ab11a/libinterp/operators/op-class.cc#l81

When a binary operator is called with one or two class arguments, the class type of the left hand argument takes precedence. There should be additional checking if they are both class types.

Can you test what Matlab does in this case if the two classes are completely unrelated (no inheritance, no superiorto declaration, just two classes that both provide plus methods)?

Mike Miller <mtmiller>
Group Member
Fri 11 Jul 2014 11:30:50 PM UTC, comment #2: 

Unsurprisingly, "<double> + C" seems to work.  Its just P + C that does the wrong thing.

Colin Macdonald <cbm>
Fri 11 Jul 2014 11:09:38 PM UTC, comment #1: 

also present in 3.6.4.

Colin Macdonald <cbm>
Fri 11 Jul 2014 11:08:52 PM UTC, original submission:  

I have a class called parent and a class called child.

Say P is instance of parent and C is instance of child.

parent and child have "myplus" and "plus" methods.

1)  P+C and C+P call different plus methods, independent of whether superiotor('parent') is set.  This behaviour DOES NOT match matlab.

2)  plus(P,C) and plus(C,P) call the child plus method if superiorto is set, otherwise different methods.  This behaviour matches matlab 2008a.

3)  myplus(P,C) and myplus(C,P) call the child myplus method if superiorto is set, otherwise different methods.  This behaviour matches matlab 2008a.

Tested with plus, minus, mtimes.  Tested on 3.8.1 and "hg - 1 month".

(personally, I don't see why I'd ever want the behaviour w/o superiorto, but that is not the bug here)

I'll attach my minimal example: untar and run mytest.m

Colin Macdonald <cbm>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45100:  bug-42735.patch added by siko1056 (7KiB - text/x-patch)
file #39509:  D.m added by cbm (207B - application/vnd.wolfram.mathematica.package)
file #39506:  A.m added by cbm (201B - application/vnd.wolfram.mathematica.package)
file #39507:  C.m added by cbm (181B - application/vnd.wolfram.mathematica.package)
file #39508:  B.m added by cbm (201B - application/vnd.wolfram.mathematica.package)
file #31704:  inheritance.tar added by cbm (20KiB - application/x-tar)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by siko1056
  • -email is unavailable- added by siko1056 (Updated the item)
  • -email is unavailable- added by genuinelucifer (Posted a comment)
  • -email is unavailable- added by cbm (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 21 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-05 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-10-04 siko1056 StatusIn Progress Ready For Test
    2018-10-04 mmuetzel StatusNeed Info In Progress
    2018-10-04 siko1056 StatusFixed Need Info
        Open/ClosedClosed Open
    2018-09-28 siko1056 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-09-26 mtmiller Carbon-CopyRemoved 80942 -
    2018-09-26 siko1056 StatusConfirmed Ready For Test
        Assigned toNone siko1056
        Releasedev 4.4.1
        Carbon-Copy- Added -email is unavailable-
    2018-09-26 siko1056 Attached File- Added bug-42735.patch, #45100
    2017-01-19 cbm Attached File- Added D.m, #39509
    2017-01-19 cbm Attached File- Added A.m, #39506
        Attached File- Added C.m, #39507
        Attached File- Added B.m, #39508
    2016-11-01 mtmiller StatusNone Confirmed
        Operating SystemGNU/Linux Any
    2014-07-11 cbm Attached File- Added inheritance.tar, #31704

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code