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

 
 

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

Submitter:  Hartmut <hardy>
Submitted:  Mon 11 Dec 2017 07:27:46 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  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

Tue 19 Nov 2019 06:05:56 PM UTC, comment #17: 

The issue in this bug report has been fixed again.  Closing report.

I opened a different bug report at bug #57271 that deals with other oddities when the label is an empty string or a space.

Rik <rik5>
Group administrator
Tue 19 Nov 2019 05:32:59 PM UTC, comment #16: 

I pushed the following cset to allow empty strings as leged keys:

http://hg.savannah.gnu.org/hgweb/octave/rev/6eb7491a8794

The legend item is still too close to the upper border but this is due to bug #57269.

Pantxo Diribarne <pantxo>
Group Member
Mon 18 Nov 2019 10:00:08 PM UTC, comment #15: 

The original motivating code works with Matlab, but fails with the new legend.m code.


x = 0:10;

plot(x, x+1)
legend('x+1');

hold on
plot(x, x+2);
hold off

legend('', 'x+2'); % first string is empty -> error


It should be possible to set an empty value for a legend string.

Rik <rik5>
Group administrator
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>
Group 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?


octave:2> h = plot (10:-1:1, "o-;Legend String;", -[1:10], ";Leg End String;");
octave:3> kids = get(gcf, 'Children')
kids =

  -8.3648
  -1.3944

octave:5> get(kids(1))
ans =

  scalar structure containing the fields:

    edgecolor =

       0   0   0

    interpreter = tex
    location = northeast
    orientation = vertical
    string =
    {
      [1,1] = Legend String
      [1,2] = Leg End String
    }

    textcolor =

       0   0   0

    textposition = right
    unmodified_axes_outerposition =


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


    [hplots, text_strings] = __getlegenddata__ (hlegend);
    if (isempty (hplots))
      delete (hlegend);
    else
      legend (legdata.handle, hplots, text_strings);
    endif


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


          if (have_labels)
            set (kids(k), "displayname", arg);
          endif


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


title ""
AND
notitle



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


    if (any (strcmp (typ, {"line", "patch", "surface", "hggroup"})))
      dname = get (kids(i), "DisplayName");


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:


sebald@ ~/octave/octave/octave $ grep getlegenddata */*/*/*/* -s
scripts/plot/draw/private/__errplot__.m:      [hlgnd, tlgnd] = __getlegenddata__ (hlegend);
scripts/plot/draw/private/__plt__.m:      [hlgnd, tlgnd] = __getlegenddata__ (hlegend);


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?


close all
h = plot (1:10, 'o-')
legend ('abc')
set (h, 'displayname', '')
% Does legend disappear?
double (get (h, 'displayname'))


I've asked on the mailing list about this.

I've also re-opened this bug report.

Rik <rik5>
Group 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:


octave:1> graphics_toolkit fltk
octave:2> h = plot (10:-1:1, "o-;Legend String;", -[1:10], ";Leg End String;");
octave:4> hl = legend(h, {"Line1" "Line2"});
octave:5> get(h(1),'displayname')
ans = Line1
octave:6> get(h(2),'displayname')
ans = Line2
octave:8> set (h, "displayname", "")
octave:9> get(hl,'string')
ans =
{
  [1,1] =
  [1,2] =
}

octave:10> legend(h, {})
octave:11> get(h(1),'displayname')
ans = data1
octave:12> get(h(2),'displayname')
ans = data2


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:


function updatename (h, ~, hlegend)

  ## When string changes, have to rebuild legend completely
  [hplots, text_strings] = __getlegenddata__ (hlegend);
  ## FIXME: See bug #52641.  Changing an existing legend string to a blank
  ##        can trigger this.
  if (! isempty (hplots))
    legend (get (hplots(1), "parent"), hplots, text_strings);
  else
    curstr = get (hlegend, 'string');
    curstr(:) = {''}
    set (hlegend, 'string', curstr);
  endif

endfunction


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)


warning: legend: plot data is empty; setting key labels has no effect
warning: called from
    legend at line 378 column 9
    legend>updatelegend at line 1081 column 9
    legend>updatename at line 1247 column 5
arg =
error: kids(0): subscripts must be either integers 1 to (2^63)-1 or logicals
execution error in graphics callback function


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:


%    if (! isempty (hplots))
      legend (hlegend, hplots, text_strings);
%    endif


But this produces a warning/error:


warning: legend: plot data is empty; setting key labels has no effect
warning: called from
    legend at line 378 column 9
    legend>updateline at line 1193 column 7
error: legend: expecting argument to be a string


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:


octave:20> get(h,'displayname')
ans =
{
  [1,1] =
  [2,1] =
}

octave:21> kids = get(gcf,'children')
kids =

  -10.640
  -15.349

octave:23> get(kids(1),'string')
ans =
{
  [1,1] = First Line
  [1,2] = Second Line
}


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:


    if (! isempty (hplots))
      legend (get (hplots(1), "parent"), hplots, text_strings);
    endif


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


h = plot (1:10, "o-;Legend String;");
set (h, "displayname", "")


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>
Group administrator
Mon 11 Dec 2017 07:27:46 PM UTC, original submission:  

Here is a short script to show the behavior:


clear;
close all;

graphics_toolkit qt % same error with fltk and gnuplot

x = 0:10;

plot(x, x+1)
legend('x+1');

hold on
plot(x, x+2);
hold off

legend('', 'x+2'); % first string is empty -> error


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


error: hplots(1): out of bound 0
execution error in graphics callback function


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 pantxo (Posted a comment)
  • -email is unavailable- added by rik5
  • -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)
  •  

    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 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-11-19 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2019-11-19 pantxo StatusConfirmed Ready For Test
    2019-11-18 rik5 Item GroupUnexpected Error or Warning Matlab Compatibility
        StatusFixed Confirmed
        Open/ClosedClosed Open
        Carbon-Copy- Added pantxo
    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.13-3230.
    Corresponding source code