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

 
 

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

Submitted by:  Lars Kindermann <larskindermann>
Submitted on:  Sun 26 Mar 2017 10:24:47 AM UTC  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Unexpected Error
Status: Works For MeAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project 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>
Project 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>
Project Member
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.

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>
Project 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>
Project 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>
Project 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>
Project 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

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>
Project Administrator
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>
Project Administrator
Wed 29 Mar 2017 07:03:33 PM UTC, comment #25:

As another example...

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

Rik <rik5>
Project 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

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

Rik <rik5>
Project 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

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>
Project 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.

Here the results:

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>
Project Member
Wed 29 Mar 2017 04:41:25 PM UTC, comment #21:

John: yes, it passes "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>
Project 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>
Project Administrator
Wed 29 Mar 2017 04:29:49 PM UTC, comment #18:

Guillaume:

Also, what does Matlab do if you just execute

? 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>
Project 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>
Project 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:

and

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

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:

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>
Project 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>
Project 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>
Project 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>
Project 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:

and again like this:

?

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>
Project Administrator
Wed 29 Mar 2017 09:10:10 AM UTC, comment #6:

I still obtain

whereas

so still this is not fixed.

By the way: I just did

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?

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

the example failing code works.

Rik <rik5>
Project 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>
Project 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.

This method fails

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:

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

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

this works

but this crashes

Lars Kindermann <larskindermann>

 

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

Attach File(s):
   
   
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
  • -unavailable- added by siko1056 (Posted a comment)
  • -unavailable- added by siko1056
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by mmuetzel (Posted a comment)
  • -unavailable- added by ernstreissner (Posted a comment)
  • -unavailable- added by gyom (Posted a comment)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by larskindermann (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 13 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 05 Apr 2017 08:08:05 PM UTCrik5Open/ClosedOpen=>Closed
    Wed 05 Apr 2017 08:52:56 AM UTCsiko1056StatusReady For Test=>Works For Me
    Wed 05 Apr 2017 07:44:14 AM UTCsiko1056Carbon-Copy-=>Added siko1056
    Wed 29 Mar 2017 03:12:28 PM UTCernstreissnerAttached File-=>Added double.m, #40197
      Attached File-=>Added pn.m, #40198
    Wed 29 Mar 2017 01:45:43 PM UTCernstreissnerAttached File-=>Added octaverc, #40195
    Wed 29 Mar 2017 01:12:31 PM UTCernstreissnerAttached File-=>Added pn.m, #40192
      Attached File-=>Added double.m, #40193
      Attached File-=>Added display.m, #40194
    Tue 28 Mar 2017 10:31:30 PM UTCrik5StatusConfirmed=>Ready For Test
    Tue 28 Mar 2017 09:59:10 PM UTCrik5Dependencies-=>bugs #50591 is dependent
    Mon 27 Mar 2017 05:30:40 PM UTCrik5StatusNone=>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.1-cleanup1