bugGNU Octave - Bugs: bug #52641, Changing legend...

 
 

bug #52641: Changing legend "displayname" property to empty string "" leads to printing an error

Submitted by:  Hartmut <hardy>
Submitted on:  Mon 11 Dec 2017 07:27:46 PM UTC  
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  Any

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)

Wed 13 Dec 2017 07:56:22 PM UTC, comment #14:

@Dan: Can you copy and paste your comments in to the Task tracker item for this (https://savannah.gnu.org/task/?14243)? This bug report is closed and won't be viewed by everyone who might be modifying legend.m

legend.m is very, very tricky and there are a lot of hacks to get Matlab conformance. Even good ideas, like dropping "UserData", can have weird side effecs. I'm reminded of the Alexander Pope quote, "Fools rush in, where angels fear to tread". I try to avoid legend.m if I can.

Rik <rik5>
Project Administrator
Wed 13 Dec 2017 07:28:30 PM UTC, comment #13:

I looked at legend.m more deeply last night. Whoo, that's one patchwork of disparate ideas. There's storing handles for later use, state variables to prevent processing callbacks, persistent variables for the same purpose. This really could use an overhaul. If one wanted to stick to the callback idea, I'd suggest

1) Drop the use of "userdata" everywhere. Not only is it legend.m where this "userdata" variable is being used to store things.

2) Make the listeners more abundant and at the same time smaller hunks. That is the typical strategy of callback-based frameworks. The behavior lies in all the connections made. For example, rather than reconstructing the legend via calling legend() again and again, there could be listeners tied to the axes children list. If the children list changes (due to addition of another plot item, e.g., "hold on") it could trigger a reconstruction of the plot. Listeners could be tied to the name text objects such that if one of those text objects changes--either directly or via "set(hax, 'displayname', 'newname')" which in itself triggers some callbacks--it will trigger a re-layout of the plot.

3) The way that the current-axes and legend-handle are derived seems far more complex than it needs to be. If the input is a valid axes handle, use that, otherwise gca() does the job, even constructing the axes if they don't exist. All that searching of the ancestor tree should be replaced by something far more simple.

4) With regard to the reoccurring scanning of ancestors and kids, it might be better to place more information in the list of arguments to listeners. It makes the number of variables passed around greater, but at the same time it saves running a hunk of code to reconstruct that info.

Question, isn't the legend supposed to be associated with an axes rather than a figure?

Dan Sebald <sebald>
Wed 13 Dec 2017 02:31:52 AM UTC, comment #12:

OK, then rather than UserData, this legend code could have used some other name and made it private. Not that important.

However, I'm still wondering about this solution. In the years I've been programming and as frameworks have evolved, I've grown to not like setting some variable on/off in anticipation of some loopback mode. Often some bug shows up that is a strange combination of circumstances.

It seems to me the major issue here is that updateline() calls legend() rather than something more straightforward, i.e.,

which in turn recursively sets the parents' "displayname" in the legend() main function:

Therein lies the problem. There is no need for updateline() to eventually set some "displayname" because updateline is a callback originating from one of the parent plot objects, correct? It's not the legend() initiating updateline(), it is the parent. Furthermore, if updateline() was called--being a callback from some existing object--the only thing that can happen is that "displayname" is updated for the text-object (indicated by the passed-in handle via the listener) or it is empty in which case that means simply deleting the associated legend objects from the list. We could in fact pass both the line (surface, patch) icon handle and the text handle via the listener. So, is there ever a case where an updateline() needs to completely reconstruct the legend by calling legend()? If so, we could still keep the construction as a separate routine and have only the setting of parent "displayname"s in the true legend() subroutine.

Dan Sebald <sebald>
Tue 12 Dec 2017 10:12:25 PM UTC, comment #11:

It's true that the Matlab convention is to reserve "UserData" for users, but it is only a convention. In this case, Octave mimics a true C++ legend object by adding properties, listeners, and data on top of an axes object. Octave has been treating the legend as an opaque object for years, it already stores the axes handle to which the legend belongs in the "UserData" property.

Given that history, it was easier to add to the existing implementation rather than create a new one.

There is a task to rewrite legend.m: https://savannah.gnu.org/task/?14243. When legend() is overhauled that would be a good time to ditch the use of "UserData".

Rik <rik5>
Project Administrator
Tue 12 Dec 2017 09:58:47 PM UTC, comment #10:

So the rule then is that if there is no name associated with a graphics object on the plot that it makes little sense to include it in the key? That would be consistent with the current behavior of _getlegenddata_ which exclude entries with empty strings. I can see that as being the rule; it might be worth adding such language to "help legend" or wherever "displayname" is discussed, i.e., empty strings in the list exclude associated graphics items from the key.

The change you've made can't be done using "UserData" in the manner it does. "UserData" is reserved for use only by users, hence its name. A lot of times when users program a GUI, they may use the UserData as a place to store flow-state, button settings, etc. In the case of a legend, what's to store? Don't know, previous settings? Whatever the programmer can dream of I suppose. UserData should be a default of [] and only left up to the external script programmer to manage.

Dan Sebald <sebald>
Tue 12 Dec 2017 09:08:23 PM UTC, comment #9:

I checked in a new change that disables callbacks during the construction of a legend object (http://hg.savannah.gnu.org/hgweb/octave/rev/b9462090773a). This is more efficient, and takes care of the issue reported here. After running a test in Matlab, i appears that empty labels are not displayed in a legend. I fixed the callback updateline() in legend.m to implement that. I tested with all three toolkits and the various fixes work. Because empty legend strings are not pushed forth from Octave, I don't think there needs to be any change to the gnuplot interface to distinguish between

Rik <rik5>
Project Administrator
Tue 12 Dec 2017 06:10:33 PM UTC, comment #8:

Well, think about it a bit. It sort of does make sense for updatename to be its own function. Each of these objects in the legend has its own name. In fact, the _getlegenddata_ main loop is constructed as

which is somewhat generic.

And if there is an updatepatch, updatesurface (which is a good idea) it's likely that its code for updating its name will be a replica of the code under "if (update_name)" in the updateline callback. So, updatename callback can be used for all these different graphics object types just using a different handle.

I don't see many places outside _getlegenddata_.m and legend.m where _getlegenddata_ is used. There are only:

For the _errplot_.m routine, it just tags some extra items at the end of the current hlgnd and tlgnd and calls legend() to set the new names. In the case of _plt_.m, it's pretty much the same inside the _plt_key_ subfunction. So, there really isn't any case where hlgnd or tlgnd are used in a conditional way that might cause an error. I don't think there would be any new issues with a change to _getlegenddata_. I wonder if anyone has actually generated plots in which the strings are empty.

In fact, I was trying to imagine how the _getlegenddata_ could be moved inside of legend.m and re-used in some way. Inside legend.m there are a bunch of loop constructs that are almost identical to what _getlegenddata_ is doing, i.e., testing for "surface", "patch", "line" inside some loop. Not critical, but just good coding practice to reuse things.

Also, perhaps there is some way to add plot objects to legends without first calling _getlegenddata_ and then appending to the end of the list--although, it is convenient to do so. But still, the convention seems to be

hold on
SOMEPLOTFUNC(...,'displayname','keyname')

So, perhaps _errplot_.m can use that route instead.

Dan Sebald <sebald>
Tue 12 Dec 2017 05:11:01 PM UTC, comment #7:

@Dan: I don't think it makes sense to add a new function updatename. The function updateline is the callback for all line objects which have a legend. Although we don't have it implemented, there should be callbacks for patch and surface objects as well. Logically, those callbacks should be called updatepatch and updatesurface and they will contain the segregated code for dealing with each of those objects respectively. Dividing by property, such as "color" or "name" would also have been a possibility, but this would have required a lot of callbacks since a line object has 8 properties and the other objects might have added even more. Instead, we only require 3 callbacks--one per object type.

The performance issue is that recursion should be stopped. Unfortunately moving the code for updating the strings to its own callback will not prevent that. It would be possible to use a global variable, or the "userdata" property of the legend object itself, as an out-of-band communication method.

I am very reluctant to change _getlegenddata_ because it is used in more routines than just legend.m, and the legend code itself is very complicated with potential for unexpected side effects.

It seems that there is another in-band/out-of-band communication problem. How is Octave supposed to distinguish between an object which has never been labeled, for which DisplayName is "", and an object for which the label is desired to be blank? One possibility is to change labels that are explicitly set to "" to contain one space, " ". I think we need some testing from someone with access to Matlab.

What does this code do?

I've asked on the mailing list about this.

I've also re-opened this bug report.

Rik <rik5>
Project Administrator
Tue 12 Dec 2017 08:06:10 AM UTC, comment #6:

OK, I think I found the missing piece. It lies in what I posted previously: "something about not finding objects that can be labeled in a legend (but this shouldn't be the case, it should know there is still something present, i.e., lines)". That information is constructed in _getlegenddata_.m. I altered that function so that it constructs the hplots list even if "displayname" is empty. See the attached patch.

Try things like:

Note that after the changes made there is a bug in gnuplot toolkit. When the titles are empty, gnuplot doesn't draw the legend correctly. Instead of

title ""

the option should be

notitle

I'll wait until this bug is settled before opening a new bug report for the gnuplot toolkit issue.

(file #42618)

Dan Sebald <sebald>
Tue 12 Dec 2017 05:40:27 AM UTC, comment #5:

I tried something like the following:

but it still produces an error, and the reasons seems to be something about not finding objects that can be labeled in a legend (but this shouldn't be the case, it should know there is still something present, i.e., lines)

This one looks like tricky loop-back strangeness.

Dan Sebald <sebald>
Tue 12 Dec 2017 04:23:46 AM UTC, comment #4:

The source of the original error is this hunk of code

get (hplots(1), "parent")

either in and of itself or because it generates something that is not a valid handle. But we know the legend handle already, so I changed that conditional test to:

But this produces a warning/error:

There is an inherent issue here in the fact that the legend name objects have disappeared without having cleared or redrawn the "Legend String" contents. Since there is nothing to be redrawn, the plot graphics don't change.

Either the legend needs to retain some form of string (an emtpy string "") that will be redrawn, or the legend code needs to know that if the name strings are empty that a clear needs to be done. It doesn't seem logical, as far as the latter case. Somewhere when the "displayname" is run it needs to recognize the legend name is changing so it has to redraw, regardless of the empty string.

Perhaps gnuplot has it right somehow in that regard, but I don't think intentionally. Hmm, it seems to me the underlying problem is that there are two locations containing the same information. Note that after the error above, I look at the contents:

There is a circular problem here. If one changes the figure "displayname", that has to trigger a change in the legend's "string". If one changes the legend, that has to trigger a change to the figure's "displayname". And if there is a redraw in both locations, it complicates matters. If this information were contained in only one location, perhaps it would make it more tractable. I'll think about this a bit.

Dan Sebald <sebald>
Tue 12 Dec 2017 02:42:58 AM UTC, comment #3:

Actually, gnuplot is updating the "displayname" somewhere else too because I can comment out all the lines of "updatename" listener and the "Legend String" goes away. So gnuplot toolkit is covering up the issue.

Anyway, I see what the issue is here. The original error example works, but not your second, simpler example. But your second example uses the "displayname" listener callback method. However, if hplots is empty, the legend string will not be updated:

Dan Sebald <sebald>
Tue 12 Dec 2017 02:24:38 AM UTC, comment #2:

Rik, you are saying that

http://hg.savannah.gnu.org/hgweb/octave/file/a7dfb685d261/scripts/plot/appearance/legend.m#l415

is inefficient because it recursively calls the routine it exists in?

I'm attaching a simple mod for you to consider that gets rid of the true/false input to updateline and replaces it with a new updatename listener. That seems more efficient, as keeping callback simple is typically good practice.

Also, this modification (after the patch you created and after the attached patch is applied) works for me in gnuplot, but it doesn't work in the FLTK/Qt toolkits. I've verified that the callback is occurring, so I don't know what the issue is. For Qt/FLTK the legend text remains at "Legend String".

(file #42614)

Dan Sebald <sebald>
Mon 11 Dec 2017 10:52:32 PM UTC, comment #1:

Confirmed. The problem is with any null string and a simpler example is

I fixed the problem on the stable branch here (http://hg.savannah.gnu.org/hgweb/octave/rev/a7dfb685d261). This issue exposed a big inefficiency on the legend code. I have not dealt with that, but noted it in a FIXME comment in the code. Maybe it doesn't matter too much because legend() isn't called a lot and the response of Octave can be on human time scales (100's of milliseconds).

Rik <rik5>
Project Administrator
Mon 11 Dec 2017 07:27:46 PM UTC, original submission:

Here is a short script to show the behavior:

This code produces the following error message under Octave (4.2.1 release and current dev branch):

Once I change the first (empty) string in the last legend command to something non-empty, then the error message dissappears.

Running the same code in Matlab (R2017b) does NOT throw any error message.

The resulting figure looks fine to me, for both Octave and Matlab.

Changing the graphics toolkit to fltk or to gnuplot both still gives the very same error.

Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by sebald (Updated the item)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by hardy (Submitted the item)
  •  

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

    Only project members can vote.

     

     

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-12-12 rik5 StatusIn Progress => Fixed
        Open/ClosedOpen => Closed
    2017-12-12 rik5 StatusFixed => In Progress
        Open/ClosedClosed => Open
    2017-12-12 sebald Attached File- => Added octave-updateline_listener_simplify-djs2017dec12.patch, #42618
    2017-12-12 sebald Attached File- => Added octave-updateline_listener_simplify-djs2017dec11.patch, #42614
    2017-12-11 rik5 StatusNone => Fixed
        Open/ClosedOpen => Closed
        Summary'legend' + 'hold on' produces unnecessary error => Changing legend "displayname" property to empty string "" leads to printing an error

    Back to the top


    Powered by Savane 3.3