bugGNU Octave - Bugs: bug #49980, plotyy fails after previous plot

 
 

bug #49980: plotyy fails after previous plot

Submitter:  None
Submitted:  Tue 03 Jan 2017 10:16:09 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
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

Sat 07 Jan 2017 12:05:27 AM UTC, comment #15: 

@pantxo: I checked in the last cset here (http://hg.savannah.gnu.org/hgweb/octave/rev/5dca1727ce94).

I made a few changes.  It appears that _go_draw_axes_ in graphics.cc is completely unused, so I deleted it.  It also appears that axes::properties::set_defaults is never called without mode == "reset".  I put in an assert statement to guarantee that.  If there are no complaints then in 6 months we can delete the code paths that are testing for it.

Rik <rik5>
Group administrator
Fri 06 Jan 2017 04:30:59 PM UTC, comment #14: 

Yes, I was going to write back and suggest this strategy because newplot.m does know about the state of the axes.  I will try and review the latest patch sometime later today.

Rik <rik5>
Group administrator
Fri 06 Jan 2017 09:51:58 AM UTC, comment #13: 

Now I see there is a much simpler alternative than the one I described in comment #12. The way we generally use newplot is as follows (excerpt from plot.m):


[hax, varargin, nargs] = __plt_get_axis_arg__ ("plot", varargin{:});

  if (nargs < 1)
    print_usage ();
  endif

  oldfig = [];
  if (! isempty (hax))
    oldfig = get (0, "currentfigure");
  endif
  unwind_protect
    hax = newplot (hax);


Which means we expect newplot to create the new axes/figure itself if it doesn't already exist. The simple approach is thus if newplot created the axes, then it knows it hasn't got to reset it. I attached a modified patch that implements this approach.

Now there is still the case


close all
axes ()
plot (1:10)


In this case newplot will still reset already clean figure/axes but I think we can live with it. Thoughts?

(file #39389)

Pantxo Diribarne <pantxo>
Group Member
Thu 05 Jan 2017 09:46:07 PM UTC, comment #12: 

There could be a hidden base_property, e.g. "__is_clean__", that Freset can use to decide what to do.
At first sight, set, addproperty and addlistener (and the internal _go_axis_init_) are the only functions where one can alter a graphics object from the interpreter; but there may be others and we need to identify everyone... Those functions should take care of turning "__is_clean__" off, but only if necessary (set can return without error and without having actually changed any value).

At the end of the day the question is "is calling reset on eventually clean axes such an overhead?" At least we have lived with it up to now.

Pantxo Diribarne <pantxo>
Group Member
Thu 05 Jan 2017 06:24:44 PM UTC, comment #11: 

I applied the first patch to stable after fixing the reject errors (http://hg.savannah.gnu.org/hgweb/octave/rev/14f1738a56ae).  That takes care of the regression in this bug report.

On the dev branch, I modified your patch slightly.  Instead of calling


cla (ca, "reset")


from within newplot.m, I changed the code to directly issue the equivalent commands


delete (allchild (ca));
reset (ca);


This is a performance enhancement because all of the first part of cla.m is input validation and trying to determine which axes to act on before issuing the above commands.  Since we already know the axes in question, there is a significant time savings to just issue the commands.

Also, I added


base_graphics_object::remove_all_listeners (void)
 {
+  std::cerr << "Executing remove_all_listeners\n";


just to see what is happening during plotting.


octave:1> close all
octave:2> plot (1:10)
Executing remove_all_listeners
Executing remove_all_listeners
Executing remove_all_listeners
Executing remove_all_listeners
Executing remove_all_listeners


So it seems that newplot.m needs to be altered to understand when a brand new axes has been created (in which case values are already at defaults) and when the axes of an existing plot is being modified.

@Pantxo: I'm uploading the latest version of the patch as rm_listeners3.cset so you can start from there.


(file #39370)

Rik <rik5>
Group administrator
Thu 05 Jan 2017 01:59:53 PM UTC, comment #10: 

Sorry, the previous comment should read "should not rely on m listeners.

Pantxo Diribarne <pantxo>
Group Member
Thu 05 Jan 2017 01:58:21 PM UTC, comment #9: 

Whatever the outcome I still think that the first patch is worth applying: IMHO the behavior of base graphics objects like axes should rely on m listeners.

Pantxo Diribarne <pantxo>
Group Member
Thu 05 Jan 2017 09:00:09 AM UTC, comment #8: 

I was sure I had modified cla.m to use reset instead of _go_axes_init_. That is what I did in the attached updated patch.


(file #39367)

Pantxo Diribarne <pantxo>
Group Member
Thu 05 Jan 2017 05:22:02 AM UTC, comment #7: 

I tried the fourth patch in comment #6, but it fails the test code from the original poster.

This is probably because


cla reset


eventually calls the same _go_axes_init_ code.  If you look at cla.m you will find the following


  if (! do_reset)
    delete (get (hax, "children"));
  else
    __go_axes_init__ (hax, "replace");
    __request_drawnow__ ();
  endif


do_reset will be true for "cla reset" so the else branch will be taken.


Rik <rik5>
Group administrator
Wed 04 Jan 2017 09:53:55 PM UTC, comment #6: 


>> ... there is nothing in the language that requires a programmer to be that forward thinking.


Yes average users should not have to bother with this and I agree that having "newplot" reset the listeners is the way to go.

In newplot.m I see this note:

      ## delete (allchild (ca));
      ## reset (ca);
      ###########################
      ## Actually, __go_axes_init__ does both less and more.
      ## It doesn't really remove all children since it re-instantiates
      ## xlabel, ylabel, zlabel, and title text objects.
      ## Also it preserves font properties like fontsize.
      ## For the time being, in order to have axis labels and title work,
      ## the above code is required.


I don't understand why we have to use _go_axes_init_ and why it has to delete and recreate labels, does someone have any insight?

Why not something like:

cla reset


Currently "cla reset" will call axes::reset_default_properties which in turn follows approximately the same route as _go_axes_init_ so the attached cset would do the trick.


(file #39366)

Pantxo Diribarne <pantxo>
Group Member
Wed 04 Jan 2017 05:58:54 PM UTC, comment #5: 

A good coding approach, but there is nothing in the language that requires a programmer to be that forward thinking.

The audience for Octave is not principally computer programmers, and many would assume that plotting something new into an existing figure would remove all traces of the previous plot.  That is the mental model I would use.

I think the best path forward would probably be to modify the updaters patch so that it applies cleanly on stable (there were rejects when I tried).  And then on the development branch implement the code to remove all listeners.

Does that sound like an okay plan?


Rik <rik5>
Group administrator
Wed 04 Jan 2017 05:43:05 PM UTC, comment #4: 

My approach in general (see annotation.m for figure listeners) is something like this:


hax = axes ();
ht = text ();
update_txt = @(h, d, htxt) set (htxt, "string", disp (get (h, "color")));
lsn = {update_txt, ht};
addlistener (hax, "color", lsn)
del_lsn = @() dellistener (hax, "color", lsn);
addlistener (ht, "beingdeleted", del_lsn)


Pantxo Diribarne <pantxo>
Group Member
Wed 04 Jan 2017 04:40:55 PM UTC, comment #3: 

@Pantxo: Yes, using C++ updaters also works.

In general, however, shouldn't listeners be removed from axes objects when the axes is reset?  What happens if someone uses addlistener for an axes object in their own m-file?  I believe that will still cause problems which means switchi to C++ updaters is really only a partial fix.

Rik <rik5>
Group administrator
Wed 04 Jan 2017 10:29:53 AM UTC, comment #2: 

How about the attached cset which makes use of internal c++ (private) updaters instead of m listeners?
It will be API/ABI compatible and thus can be pushed to stable

(file #39364)

Pantxo Diribarne <pantxo>
Group Member
Wed 04 Jan 2017 01:44:44 AM UTC, comment #1: 

Confirmed.

The problem is that there is a listener added to the axes object which will change the color of the labels when the axes color property is changed.

When you call any graphics function, one of the first things it does it call newplot() to either get a new axes object or reuse an existing one.  If newplot decides to reuse an existing object it resets the default properties of the object.  The sequence of events is hard to follow but eventually the C++ function _go_axes_init_ in graphics.cc is called.  The issue is that the C++ function does not include removing listeners on the axes object. 

When an attempt to change the axes color property is made by plotyy the old callback fires, but the ylabel that it pointed to is gone and so you get an error.

A simpler example to reproduce the error is:


plot (1:4)
ylabel ('y')
plot (1:10)
set (gca, "ycolor", 'r')


I have two possible fixes.  The correct way to fix this is to call remove_all_listeners() on the axes graphics_object.  Unfortunately, the required function is part of the class base_graphics_object and is not exposed.  In order to call it, the API for the class graphics_object needs to have a public function remove_all_listeners which calls the base_grahpics_object function of the same name.  This would be a change to the internal API which is normally only made when Octave changes minor versions (say from 4.2 to 4.4).  I've added jwe to the CC list so that he can comment on whether it would be acceptable in this instance.  The attached file rm_all_listen.patch implements this approach.

The second approach modifies the callback function to check whether the handle is valid before calling set() to change the color.  The attached file cb_color.patch implements this method.  The problem with this is that the callback will still be called every time something modifies the color properties of the axes.  If you do a series of several plots which re-use the same axes and all have labels or titles then the callback could be invoked a lot.



(file #39360, file #39361)

Rik <rik5>
Group administrator
Tue 03 Jan 2017 10:16:09 PM UTC, original submission:  

this script


a = (1 : 10);
plot(a,a);
ylabel('y');
plotyy(a,a,a,fliplr(a));
pause


produces this console output with both dev and 4.2.0 (not with 4.0.3)


error: set: invalid handle (= -4.71108)
execution error in graphics callback function


The plotyy output looks normal.
The error is not shown when a "close all" is inserted just before the plotyy, or when the ylabel call is removed

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #39389:  rm_listeners4.patch added by pantxo (5KiB - text/x-diff)
file #39370:  rm_listener3.cset added by rik5 (3KiB - application/octet-stream)
file #39367:  remove_listeners2.patch added by pantxo (3KiB - text/x-diff)
file #39366:  remove_listeners.patch added by pantxo (3KiB - text/x-diff)
file #39364:  updatelabelscolor.patch added by pantxo (4KiB - text/x-diff)
file #39360:  rm_all_listen.patch added by rik5 (912B - application/x-download)
file #39361:  cb_color.patch added by rik5 (554B - application/x-download)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by pantxo (Updated the item)
  • -email is unavailable- added by rik5 (Could you review whether it would be okay to change graphic_object API on stable?)
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by None (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-01-07 rik5 StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2017-01-06 pantxo Attached File- Added rm_listeners4.patch, #39389
    2017-01-05 rik5 Attached File- Added rm_listener3.cset, #39370
        StatusPatch Submitted In Progress
    2017-01-05 pantxo Attached File- Added remove_listeners2.patch, #39367
    2017-01-04 pantxo Attached File- Added remove_listeners.patch, #39366
    2017-01-04 pantxo Attached File- Added updatelabelscolor.patch, #39364
        StatusConfirmed Patch Submitted
    2017-01-04 rik5 Carbon-Copy- Added jwe
    2017-01-04 rik5 Attached File- Added rm_all_listen.patch, #39360
        Attached File- Added cb_color.patch, #39361
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code