bugGNU Octave - Bugs: bug #50640, display function not passing...

 
 

bug #50640: display function not passing arguments to overloaded version of function

Submitter:  Lars Kindermann <larskindermann>
Submitted:  Sun 26 Mar 2017 10:24:47 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Works For Me Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 05 Apr 2017 09:12:51 PM UTC, comment #36: 

I found and fixed the cause of old-style classes not correctly adding a newline after calling disp().  See this cset (http://hg.savannah.gnu.org/hgweb/octave/rev/5ea6c6d0c2db).

Rik <rik5>
Group administrator
Wed 05 Apr 2017 08:52:56 AM UTC, comment #35: 

Wow, how one can miss a whole thread about an issue I was working on independently (see comment #4).

This item seems to be fixed and I added bug #50729 as suggested in comment #34. If no one minds, I close this one.

Kai Torben Ohlhus <siko1056>
Group Member
Fri 31 Mar 2017 02:43:27 PM UTC, comment #34: 

While it is still valid to overload the display method, it should be made clear that the user should overload the disp method (i.a. for reasons given by Rik in comment #32).

The code example for the display method can almost entirely be re-used for an overloaded disp method.

@Ernst: Can you open a new bug report for the mis-leading documentaion and provide a changeset?

Markus Mützel <mmuetzel>
Group administrator
Fri 31 Mar 2017 09:46:46 AM UTC, comment #33: 

Well, then one has to change the manual also:
Section 34.2 tells to overwrite display.
now it should be disp, right?

has display to do with objects any more?
I think, disp has, right?

Ernst Reissner <ernstreissner>
Thu 30 Mar 2017 12:14:59 AM UTC, comment #32: 

We're actually getting very, very close.  I switched to overloading disp, rather than display, and now Octave's built-in display function takes care of handling compact/loose format.  See this cset http://hg.savannah.gnu.org/hgweb/octave/rev/f04f32f08590.

This cset also fixed up inputParser which had been broken somewhere along the way.

The issue remaining is that the built-in display should add a newline after calling disp() for whatever object it is handling.

This works for built-in objects as shown below.


octave:2> x = magic (3);
octave:3> display (x)
x =

   8   1   6
   3   5   7
   4   9   2

octave:4> disp (x)
   8   1   6
   3   5   7
   4   9   2
octave:5>



octave:5> x = audiorecorder ();
octave:6> display (x)
x =

audiorecorder object with properties:

  BitsPerSample =  16
  CurrentSample = 0
  DeviceID = -1
  NumberOfChannels =  2
  Running = off
  SampleRate =  44100
  TotalSamples = 0
  Tag =
  Type = audiorecorder
  UserData = [](0x0)
octave:7> disp (x)
audiorecorder object with properties:

  BitsPerSample =  16
  CurrentSample = 0
  DeviceID = -1
  NumberOfChannels =  2
  Running = off
  SampleRate =  44100
  TotalSamples = 0
  Tag =
  Type = audiorecorder
  UserData = [](0x0)
octave:8>


To temporarily get things to work I added an extra newline in the disp.m routines for old-style class objects (@audioplayer, @audiorecorder, @ftp).  The new-style classdef class objects like inputParser already work correctly (why?  Who knows).

Rik <rik5>
Group administrator
Wed 29 Mar 2017 09:53:21 PM UTC, comment #31: 

Rik:

Yes, I guess there is not much point in creating both display and disp methods if display is just going to print the variable name and forward to disp, same as the built-in display function would.

On the page you linked, there is also a statement about calling display with two arguments (the first I've seen about that!) though it is not entirely clear to me when that is supposed to happen.  And, based on experiments, it seems that it is not completely accurate.

Also, that page links to http://www.mathworks.com/help/matlab/matlab_oop/custom-display-interface.html and also to the docs for the matlab.mixin.CustomDisplay class.  So there appears to be some massive complexity here, just to print stuff.

Oh, and much of that only applies to classdef classes, not the legacy classes that we have (mostly) been looking at (except for inputParser).

My conclusion:  It's hopeless.  I'm giving up and getting a real job.

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 09:38:50 PM UTC, comment #30: 

Oops, I think I fixed the build problem here:

http://hg.savannah.gnu.org/hgweb/octave/rev/9cd68be3dba8

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 09:03:48 PM UTC, comment #29: 

For a reasonable reference on disp/display see http://www.mathworks.com/help/matlab/matlab_oop/displaying-objects-in-the-command-window.html.

This seems to indicate that classes might want to override disp rather than display function.  It is display that takes care of printing out the variable name and '='.  It may also be taking care of the extra newline for format compact/loose.

Rik <rik5>
Group administrator
Wed 29 Mar 2017 08:38:33 PM UTC, comment #28: 

I cannot build any more after pulling the latest changeset.
Make fails with


make[2]: *** No rule to make target 'scripts/audio/@audioplayer/disp.m', needed by 'scripts/DOCSTRINGS'.  Stop.
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/home/kinder/usr/octave'
Makefile:26257: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/home/kinder/usr/octave'
Makefile:9996: recipe for target 'all' failed
make: *** [all] Error 2


Lars Kindermann <larskindermann>
Wed 29 Mar 2017 08:18:54 PM UTC, comment #27: 


> Is there a way to determine loose/compact format inside a .m file?


We've gone through several iterations of that.

Used to be "get(0, 'FormatSpacing')". This was removed because it's undocumented Matlab, but still seems to be supported. I don't know if TMW provides another way to get this info.

We then had "__compactformat__()". Now we have "[format, spacing] = format()".

Mike Miller <mtmiller>
Group Member
Wed 29 Mar 2017 07:52:54 PM UTC, comment #26: 

I checked in the following changeset:

  http://hg.savannah.gnu.org/hgweb/octave/rev/c052a29c1f1f

Is there a way to determine loose/compact format inside a .m file?

In any case, I'll leave further improvements to others.

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 07:03:33 PM UTC, comment #25: 

As another example...


octave:1> x = ftp ('alpha.gnu.org')
x = FTP Object
 host: alpha.gnu.org
 user: anonymous
  dir: /
 mode: binary


The phrase "FTP Object" needs to start one newline down.

Rik <rik5>
Group administrator
Wed 29 Mar 2017 07:00:57 PM UTC, comment #24: 

The original motivating example seems to work.  Should there be an extra newline somewhere in display.m?  See the following


octave:7> x = audiorecorder ()
x =
  scalar structure containing the fields:

    BitsPerSample =  16
    CurrentSample = 0
    DeviceID = -1
    NumberOfChannels =  2
    Running = off
    SampleRate =  44100
    TotalSamples = 0
    Tag =
    Type = audiorecorder
    UserData = [](0x0)
octave:8> y = magic (3)
y =

   8   1   6
   3   5   7
   4   9   2


The phrase "scalar structure containing the fields:" looks like it should be one newline down for "loose" format which is the default.

Rik <rik5>
Group administrator
Wed 29 Mar 2017 06:41:18 PM UTC, comment #23: 

OK, after thinking about it some more, I change Octave so that it never calls the display function with more than one input argument since this is not documented Matlab behavior and it was causing trouble for existing Octave classes.  I did that instead of checking whether a display function for a class is defined to accept one or two arguments and call it differently based on this determination, because that is more work and the feature is undocumented in Matlab anyway.

I modified the documentation for the built-in display function so that it doesn't mention the two-argument version, but I left it as before so that things like


display (1, 'foo')


will work when there is no overloaded display method (as they do in Matlab).

I've also fixed the display methods in Octave that I found so that they always use inputname instead of a second argument.

Here are the changesets:

http://hg.savannah.gnu.org/hgweb/octave/rev/7d4ca8c01bbb
http://hg.savannah.gnu.org/hgweb/octave/rev/b636f10ce53f

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 06:12:53 PM UTC, comment #22: 

Guillaume, which version of Matlab are you using. I am seeing different results on Matlab R2016a.

classdef class1

  methods
function display (a1)
  nargin
  class (a1)
  inputname (1)
  disp (a1)
end
  end
end



classdef class2

  methods
function display (a1, a2)
  nargin
  class (a1)
  inputname (1)
  disp (a1)
  class (a2)
  inputname (2)
  disp (a2)
end
  end
end


Here the results:

>> class1()

ans =

     1


ans =

class1


ans =

ans

  class1 with no properties.

>> c1=class1()

ans =

     1


ans =

class1


ans =

c1

  class1 with no properties.

>> class2()

ans =

     2


ans =

class2


ans =

ans

  class2 with no properties.


ans =

char


ans =

     1


ans =

class2


ans =

ans

  class2 with no properties.

Not enough input arguments.

Error in class2/display (line 9)
  class (a2)

>> c2=class2()

ans =

     2


ans =

class2


ans =

c2

  class2 with no properties.


ans =

char


ans =

     1


ans =

class2


ans =

c2

  class2 with no properties.

Not enough input arguments.

Error in class2/display (line 9)
  class (a2)


Calling "class2" without output argument calls display with one input argument which fails.
I expected calling "class2" with output argument to succeed. However, it fails on "disp(a2)" which in turn calls display with one input argument hence failing.

Btw: Matlab's internal editor warns with: "Overloading DISPLAY is not recommended." and advises to overload "disp" instead.

Markus Mützel <mmuetzel>
Group administrator
Wed 29 Mar 2017 04:41:25 PM UTC, comment #21: 

John: yes, it passes "ans":


>> testobj
ans =
     2
ans =
    'testobj'
ans =
    'ans'
        testobj object: 1-by-1
ans =
    'char'
ans


I have to say that I had never heard of display methods with two arguments and all the ones I used or wrote have a single one.

Guillaume <gyom>
Wed 29 Mar 2017 04:34:55 PM UTC, comment #20: 

I meant, fix Octave so that the behavior is compatible with what Matlab appears to be doing.

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 04:34:07 PM UTC, comment #19: 

Ernst:

The reason for the difference in Octave currently is that without an assignment to a variable, it is calling the display method with just one argument.  But if you assign to a variable, it is calling the display method with two arguments and your display method throws an error in that case.

I see now that Matlab is doing something different so that display methods that expect only a single argument will work.  I'll see if I can fix that so it is compatible.

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 04:29:49 PM UTC, comment #18: 

Guillaume:

Also, what does Matlab do if you just execute


testobj


?  Does it pass "ans" in as the second argument to display (if display is defined to take 2 arguments)?  Does it arrange for intputname(1) to return "ans"?  And, is it left to the display method to print the "ans = " line, or does Matlab do that prior to the call to the display method?

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 04:20:44 PM UTC, comment #17: 

Guillaume:  So, if I'm understanding correctly:

If the display method is defined to have 2 arguments, Matlab does call it with 2.  The second argument contains the name of the first argument, but inputname can't access the name of the second argument.

And if the display method is defined to have only one argument, Matlab calls it with 1 argument.

OK then.

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 03:18:13 PM UTC, comment #16: 

John: after fixing the calls to inputname (input 1 and 2, instead of a1, a2), I get:


>> a=testobj
ans =
     1
ans =
    'testobj'
ans =
    'a'
        testobj object: 1-by-1


and


ans =
     2
ans =
    'testobj'
ans =
    'a'
        testobj object: 1-by-1
ans =
    'char'
Error using inputname
Argument number is not valid.
Error in testobj/display (line 7)
  inputname (2)


and if I remove the call to inputname (2) entirely, I get:


>> a=testobj
ans =
     2
ans =
    'testobj'
ans =
    'a'
        testobj object: 1-by-1
ans =
    'char'
a



Guillaume <gyom>
Wed 29 Mar 2017 03:12:28 PM UTC, comment #15: 

you are right of course.

This time I tried removing the jar...

Well, it seems as if the problem remains the same.

(file #40197, file #40198)

Ernst Reissner <ernstreissner>
Wed 29 Mar 2017 02:11:41 PM UTC, comment #14: 

When I put your octaverc file in a place where Octave will execute it at startup, I get the following error:


error: javaaddpath: CLSPATH does not exist: ${project.build.directory}/${artifactId}-${version}.jar


I'd rather not have to duplicate your entire environment.

Can't you eliminate the parts of pn.m that require RNNumber and still see the problem?

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 01:45:43 PM UTC, comment #13: 

Ah, sorry John... i missed to send octaverc....
It should work now...
well, it should not work, you should see the problem.

(file #40195)

Ernst Reissner <ernstreissner>
Wed 29 Mar 2017 01:31:27 PM UTC, comment #12: 

pn(3.3) fails for me because the global variable RNNumber has the default value of an otherwise uninitialized global variable (an empty matrix).  What is it supposed to be?  Or, can you simplify the pn constructor to not require this global variable and still demonstrate the problem?

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 01:12:31 PM UTC, comment #11: 

@john: ok.
I presented what happens with pn(3.3) opposed to a=pn(3.3) below.
What you need is, i think the constructor pn.m
and display.m which I attached.
Since display invokes double, I also attached double.m.
If you need more, tell me.

(file #40192, file #40193, file #40194)

Ernst Reissner <ernstreissner>
Wed 29 Mar 2017 01:03:29 PM UTC, comment #10: 

Ernst: Probably not without seeing an actual complete example that demonstrates the problem you are seeing.

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 12:46:35 PM UTC, comment #9: 

can anyone tell me, why the problem occurs only,
if i assign the object to a variable??

Ernst Reissner <ernstreissner>
Wed 29 Mar 2017 12:36:09 PM UTC, comment #8: 

Guillaume: Sorry for the typo.

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 12:34:20 PM UTC, comment #7: 

Guillame:  In response to your comment #3, what does Matlab do if you define a display method for a class like this:


function display (a1)
  nargin
  class (a1)
  inputname (a1)
  disp (a1)
end


and again like this:


function display (a1, a2)
  nargin
  class (a1)
  inputname (a1)
  disp (a1)
  class (a2)
  inputname (a2)
  disp (a2)
end


?

Based on other comments that I remember seeing (in another report?) I think they both might work, and nargin will be 1 in the first case and 2 in the second.  If so, then I think the thing to do is check the number of arguments that the display method expects and if it is greater than or equal to 2, call it with the object and the variable name so that they display method can handle all of the output (including the "NAME =\n\n" part, if it chooses to do that).  Otherwise, call it with just the object as a single argument.

John W. Eaton <jwe>
Group administrator
Wed 29 Mar 2017 09:10:10 AM UTC, comment #6: 

I still obtain


pn(3.3)
ans =
 3.3000


whereas


a=pn(3.3)
error: Invalid call to display.  Correct usage is:

 -- Function File:  display(NUM)


so still this is not fixed.

By the way: I just did

hg -v pull
hg -v update


to my local version and then rebuild.
Hope this is correct.





Ernst Reissner <ernstreissner>
Wed 29 Mar 2017 08:15:26 AM UTC, comment #5: 

I saw the same problem with my self-written class pn....
Well, maybe this occurs when classes are in the game in general?

Ernst Reissner <ernstreissner>
Tue 28 Mar 2017 10:31:30 PM UTC, comment #4: 

Has this bug been fixed by this cset for bug #49794?


changeset:   23330:1eb2706a6125
user:        Kai T. Ohlhus <k.ohlhus@gmail.com>
date:        Tue Mar 28 11:43:43 2017 +0200
summary:     Handle second argument in classes with overridden display function (bug #49794).


As of today (3/28/17) using cset


parent: 23331:665f0a73429a tip
 maint: Use Octave coding conventions in cset 1eb2706a6125.


the example failing code works. 

Rik <rik5>
Group administrator
Tue 28 Mar 2017 10:46:13 AM UTC, comment #3: 

How can we check what Matlab does here?

From what I've seen, the display method has a single input argument and one can use inputname(1) to get the name of the variable. This returns "ans" when the input does not have a name (inputname's help says it returns "" in this instance but it is not the case; maybe something specific for methods).

Guillaume <gyom>
Mon 27 Mar 2017 06:23:42 PM UTC, comment #2: 

For me, nargin is 2.

I broke this when I reworked the way that display works in an attempt to improve compatibility.

I'm not sure what is supposed to happen here.  Should the name of the variable be passed to the display method so that it can display that, or should that be displayed by Octave, and the display method should only display the value?

Since we are aiming for compatibility, how does Matlab work?

John W. Eaton <jwe>
Group administrator
Mon 27 Mar 2017 05:30:40 PM UTC, comment #1: 

Confirmed.

Something is wrong with the interaction between the display routine (written in C++ at libinterp/corefcn/pr-output.cc) and the overloaded display routine for an audiorecorder object which is at scripts/audio/@audiorecorder/display.m.

If I set a breakpoint in display.m, I see that this calling method works.


rec = audioplayer();
display (rec)


This method fails


rec = audioplayer();
rec


The display.m function is called in this case, but the class object which is expected to be the first argument to the function is not passed on.  The first part of display.m is:


function display (recorder)

  if (nargin != 1)
    print_usage ();
  endif


In the failing case, nargin == 0 and Octave calls print_usage() to emit an error.

Rik <rik5>
Group administrator
Sun 26 Mar 2017 10:24:47 AM UTC, original submission:  

this works


recorder=audiorecorder();


but this crashes


recorder=audiorecorder()

error: Invalid call to display.  Correct usage is:

 -- display (RECORDER)

Additional help for built-in functions and operators is
available in the online version of the manual.  Use the command
'doc <topic>' to search the manual index.

Help and information about Octave is also available on the WWW
at http://www.octave.org and via the help@octave.org
mailing list.


Lars Kindermann <larskindermann>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #40197:  double.m added by ernstreissner (1KiB - application/ecmascript)
file #40198:  pn.m added by ernstreissner (4KiB - application/ecmascript)
file #40195:  octaverc added by ernstreissner (3KiB - application/octet-stream)
file #40192:  pn.m added by ernstreissner (4KiB - application/ecmascript)
file #40193:  double.m added by ernstreissner (1KiB - application/ecmascript)
file #40194:  display.m added by ernstreissner (2KiB - application/ecmascript)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by siko1056
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by ernstreissner (Posted a comment)
  • -email is unavailable- added by gyom (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by larskindermann (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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-04-05 rik5 Open/ClosedOpen Closed
    2017-04-05 siko1056 StatusReady For Test Works For Me
    2017-04-05 siko1056 Carbon-Copy- Added siko1056
    2017-03-29 ernstreissner Attached File- Added double.m, #40197
        Attached File- Added pn.m, #40198
    2017-03-29 ernstreissner Attached File- Added octaverc, #40195
    2017-03-29 ernstreissner Attached File- Added pn.m, #40192
        Attached File- Added double.m, #40193
        Attached File- Added display.m, #40194
    2017-03-28 rik5 StatusConfirmed Ready For Test
    2017-03-28 rik5 Dependencies- bugs #50591 is dependent
    2017-03-27 rik5 StatusNone Confirmed
        Summaryaudiorecorder() crashes when invoked without semicolon display function not passing arguments to overloaded version of function

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code