bugGNU Octave - Bugs: bug #37290, gnuplot / demo plotyy followed by...

 
 
Show feedback again

bug #37290: gnuplot / demo plotyy followed by sombrero errors out

Submitted by:  Philip Nienhuis <philipnienhuis>
Submitted on:  Thu 06 Sep 2012 08:35:17 PM UTC  
 
Category: Plotting with gnuplotSeverity: 2 - Minor
Priority: 5 - NormalItem Group: Regression
Status: NoneAssigned to: None
Originator Name: Philip NienhuisOpen/Closed: Open
Release: devOperating System: Any

Add a New Comment (Rich MarkupRich 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)

Fri 17 May 2013 04:02:45 AM UTC, comment #31:

Well, I'm not sure I've accomplished much other than fixing that small bug with setting defaults. I.e., this result:

The attached patch will clear this error, but it's too simple and I'm not sure it agrees with what you are describing Ben. Here is a sample of what it produces:

So, it sort of behaves the way defaults should, but notice the "axes" probably should not be in the name in this case, i.e., it should read "defaultlinewidth", not "defaultaxeslinewidth". Now this might be a fairly cosmetic thing, but one has to think of where to handle that: in the map itself (i.e., the property is placed in the defaults map as "defaultlinewidth"), or when the map is displayed (i.e., the property name is parsed and "axes" extracted).

Before going that route, I think it might be good to think about this for a few days. It's probably the case there is too much to tackle in the time frame of a GUI release, and in fact this might make a good Google Summer project with a mercurial branch. I can't quite understand it all right now. The root_figure seems solid--at the 0 level is defined all these factory settings, every property and value is there. And the macros contain all information about possible properties and their possible settings. But at the other end the properties are gotten via simple functions that return values. But if I try to use, say "defaultlinelinewidth" at the "axes" level to set that child property value I'm not sure if the user has set that property value directly. Going backward through the parents seems easier, i.e., if linewidth not mapped, look to parent and if defaultaxeslinewidth not mapped, look to parent and if defaultfigureaxeslinewidth not mapped, look to factoryfigureaxeslinewidth. But that too I'm not sure about.

I'm going to hold off for a few days, but what I might try for is just setting the property values back to the root_figure factory/default when a "reset(obj)" is done. For the next release, two ends of the hierarchy sort of working correctly is probably better than having an intermediate level overriding something it shouldn't.

(file #28098)

Dan Sebald <sebald>
Fri 17 May 2013 12:46:48 AM UTC, comment #30:

To set the default for a figure's children, use the same syntax as for the root, but change the object's handle.

Same for axes, but don't forget to "hold all", or else the axes property will be reset.

Ben Abbott <bpabbott>
Project Member
Thu 16 May 2013 11:58:06 PM UTC, comment #29:

Yes, I just figured that out by trying:

>> set(0, "defaultaxeslinewidth", 2.3)
>> plot(1:50)


Small problem here:

>> set(gca, "defaultlinewidth", 2.3)

error: invalid line property 'width'

>> set(gca, "linewidth", 2.3)


OK, I understand it now.

Thanks

Dan Sebald <sebald>
Thu 16 May 2013 11:49:48 PM UTC, comment #28:

I think what is supposed to happen is

sets the global default and

sets the default for the given figure handle.

http://www.mathworks.com/help/matlab/creating_plots/setting-default-property-values.html

John W. Eaton <jwe>
Project Administrator
Thu 16 May 2013 11:22:11 PM UTC, comment #27:

Oh, OK. And I see there is a way to set defaults:

xproperties is the current properties for the figure, i.e., when a conventional "set(gca, "linewidth", 2.3)" is done.

default_properties, I'm guessing, is supposed to be the default for a new figure or the result of "reset". It is controlled via "set(gca, "defaultlinewidth", 2.3)", for example, and these might be commands in one's .octaverc file, perhaps?

But maybe what is missing is a copy of factory settings into "default_properties" at the very beginning, i.e., initialization of root_figure. I'm wondering if that is what reset_default_properties() was intended to do. But there would also need to be a "reset_properties ()" that copies the "default_properties" into "xproperties" which is called when someone creates a new plot or uses "reset".

So that would give the user the ability to create his or her own default properties in .octaverc thereby overriding the factory defaults.

Should the following maybe be declared static?

so that the default_properties do not change from new figure to new figure? (I realize you might not know how the answer to that.) Otherwise when a new figure is created there is no option but to get "default_properties" from the factory defaults. That doesn't save much work though because then the user would have to do "set(gcf, "defaultXYZ", xyzval)" after every new figure, in which case one might as well do "set(gcf, "XYZ", xyzval)" after every new figure.

This was probably some loose end that was overlooked in some huge changeset.

Dan Sebald <sebald>
Thu 16 May 2013 10:18:26 PM UTC, comment #26:

I think the way that is supposed to work is (for example):

John W. Eaton <jwe>
Project Administrator
Thu 16 May 2013 09:56:14 PM UTC, comment #25:

Oh, the factory settings are currently associated with the kit. So,

Do we want the other graphics objects to display their factory defaults (a subset of the result above)?

Dan Sebald <sebald>
Thu 16 May 2013 09:13:28 PM UTC, comment #24:

Just noting a more basic incorrect result. I saw in the code:

so I tried these inputs and I'm seeing:

The missing defaults we knew about, but factory defaults isn't right either.

Dan Sebald <sebald>
Thu 16 May 2013 08:04:08 PM UTC, comment #23:

OK, I see now that the argument is being modified. Maybe it would be clearer to me if it were writtin in a more functional style instead of modifying the reference. So pass in the current value (const) along with the factory values (also const), and then return the new value, leaving it up to the caller to reset the default_properties in the graphics object.

Is the current design my fault? If so, sorry about that.

John W. Eaton <jwe>
Project Administrator
Thu 16 May 2013 07:53:31 PM UTC, comment #22:

I was thinking it was something that simple, but it is slightly more difficult. The "new_defaults" in the general function:

can't tell where it is supposed to be getting its settings from. So I think this needs to be broadened to something like

and factory_defaults plays the roll of what new_defaults was supposed to be doing. That would mean using the contrapositive for the conditional test, i.e., copy all settings onto default_properties except those that are supposed to remain the same.

I'll have a patch in a couple hours.

Dan Sebald <sebald>
Thu 16 May 2013 07:28:24 PM UTC, comment #21:

Do we need to distinguish between default and factory properties?

The simple fix seems to be to change all of the functions like

to

But if we need to be able to reset to current defaults and reset to factory defaults, we'll need separate sets of functions, or a paramter that allows us to choose which action to take.

Also, since these functions are all likely to follow exactly the same pattern, maybe they should be automatically generated by the genprops.awk script.

John W. Eaton <jwe>
Project Administrator
Thu 16 May 2013 07:16:15 PM UTC, comment #20:

OK, I found where those are coming from. There is a BEGIN_PROPERTIES macro in graphics.in.h. That should get me somewhere.

Dan Sebald <sebald>
Thu 16 May 2013 07:08:40 PM UTC, comment #19:

> The description "Remove any defaults set for the handle var{h}." is also not clear to me.


Yes, I think that is where the confusion is. There is some subtle interpretation here between the default settings and factory settings that I'm not seeing and isn't quite right in the code. I think "defaults" means what the graphics object will use without any other specification. Rik gave this example:

So the default linewidth for the axis object is now 2.3, but I think a user could go through and tweak individual lines.

Maybe a more accurate way of stating it is "Set defaults back to factory settings".

Dan Sebald <sebald>
Thu 16 May 2013 07:00:50 PM UTC, comment #18:

OK, I'll make a note of that...

I'm sort of remembering this now. I can describe what the problem is, but I don't know how to solve it because I don't know where the defaults are supposed to be coming from. (Maybe that's what "factory_defaults" is supposed to be.)

The problem is that this hunk of code in graphics.cc:

never goes inside the loop because "default_properties" is empty. I've verified that the for-loop is reached, but nothing happens. The function makes sense: new_defaults is supposed to be a copy of factory_defaults(?) and then the current settings for figure->"position", figure->"units", figure->"windowstyle", figure->"paperunits", axes->"position" and axes->"units" are copied if they are present because those are supposed to be retained.

However, I can't find where "default_properties", or "new_defaults" for that matter, is being set anywhere. It seems like there should be a ": default_properties(factory_defaults)" in the constructor and maybe:

property_list new_defaults = factory_defaults;

or something like that. I'm not sure though because those are supposed to come from the particular graphics engine, or maybe from the base class.

Dan Sebald <sebald>
Thu 16 May 2013 06:43:35 PM UTC, comment #17:

The intent may have been to allow resetting individual properties. Maybe it would not be hard to do that.

The description "Remove any defaults set for the handle @var{h}." is also not clear to me.

Matlab has a reset function that resets most properties for the handle H to their "factory" defaults. If that is not what Octave's reset function is doing, we should probably fix it, since that is the behavior that most people will expect.

John W. Eaton <jwe>
Project Administrator
Thu 16 May 2013 06:24:52 PM UTC, comment #16:

I'm looking at this one again to see if we can close this one out. The first thing I notice is this inconsistency in the documentation:

has a second input, but second inputs aren't allowed in graphics.cc:

What is the case here? Just a typo and "PROPERTY" should be removed? Or more code needs to be added to graphics.cc to handle some missing behavior?

Dan Sebald <sebald>
Mon 10 Sep 2012 07:03:29 PM UTC, comment #15:

I believe the problem is on the "set" side of things, not the "reset" side. I've followed the code that does the reset and it appears to be run, but there isn't anything in the list of defaults. I've looked at the property_list "set" for the defaults, and that doesn't appear to happen. So whatever mechanism existed before for setting the defaults isn't happening.

Dan Sebald <sebald>
Mon 10 Sep 2012 04:58:40 PM UTC, comment #14:

Yes, I see the same behavior.

The issue is now with the last plotyy demo.

Example code:

Maybe also check bug #35511 which indicates that the reset function on a graphics handle is broken.

Rik <rik5>
Project Member
Mon 10 Sep 2012 04:20:13 PM UTC, comment #13:

I'm reopening this bug report becuase it doesn't seem to be comppletely fixed yet.

I no longer see an error, and the test

does appear to work. However, if I do

+vertabim+
demo plotyy
-verbatim-

and step through all the demos, then do

I see the attached figure, not the sombrero I was expecting. So it seems that plotyy is still seting some properties that are not being cleared before creating the sombrero plot. Does anyone else see this behavior?

(file #26522)

John W. Eaton <jwe>
Project Administrator
Sun 09 Sep 2012 07:14:33 PM UTC, comment #12:

Opps!

My bad. I attributed Dan's patch to Philip

Ben Abbott <bpabbott>
Project Member
Sun 09 Sep 2012 06:53:24 PM UTC, comment #11:

Thanks for fixing, Ben & Dan.
But why is the patch attributed to me? I only raked up the problem...

Philip Nienhuis <philipnienhuis>
Project Member
Sat 08 Sep 2012 06:57:59 PM UTC, comment #10:

Thanks.

I've pushed the changeset

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

Ben Abbott <bpabbott>
Project Member
Sat 08 Sep 2012 01:59:03 AM UTC, comment #9:

Looks good here. The demo/sombrero combination passes and "test rmappdata" passes all tests.

On the "setappdata()" side of things there are tests for existence, etc. So, I guess the only thing is whether you want to use the existing "isappdata()" in some way, but I think the patch is more efficient.

Thanks,

Dan

Dan Sebald <sebald>
Sat 08 Sep 2012 01:36:30 AM UTC, comment #8:

Dan, I found a few short comings in your patch and added/modified tests.

Have I missed anything?

(file #26505)

Ben Abbott <bpabbott>
Project Member
Sat 08 Sep 2012 12:28:40 AM UTC, comment #7:

I think that the demo creates a figure with multiple handles in a way that exercised a route that hasn't been tested as of yet. Attached is a patch that fixes a spelling error, _ploty_axes_, and also first checks that the _appdata_ field exists before proceeding.

Thanks for reporting this bug.

(file #26504)

Dan Sebald <sebald>
Fri 07 Sep 2012 07:42:11 PM UTC, comment #6:

I couldn't reproduce the original, but Comment #1 does cause problems on the latest development code.

octave:1> demo ("plotyy", 1)
...
octave:2> sombrero
error: invalid conversion from real matrix to real scalar
error: _calc_dimensions_: expecting graphics handle as only argument
error: called from:
error: /usr/local/src/octave/octave/build-nogui-8/../octave/scripts/plot/private/__go_draw_axes__.m at line 55, column 8
error: /usr/local/src/octave/octave/build-nogui-8/../octave/scripts/plot/private/__go_draw_figure__.m at line 173, column 19
error: /usr/local/src/octave/octave/build-nogui-8/../octave/scripts/plot/__gnuplot_drawnow__.m at line 86, column 5

That's a start.

Dan Sebald <sebald>
Fri 07 Sep 2012 07:24:34 PM UTC, comment #5:

My cases have all been while using fltk
and on Fedora (currently fc17).

Michael Godfrey <godfrey>
Fri 07 Sep 2012 07:10:45 PM UTC, comment #4:

Vague or not, it might help to emphasize that I couldn't reproduce the original issue with fltk. (I can only vouch for my own PC's, of course).
Did you encounter your problem with gnuplot, fltk or both?

Philip Nienhuis <philipnienhuis>
Project Member
Fri 07 Sep 2012 06:50:19 PM UTC, comment #3:

This report does seem vague, but
I have noticed that there are conditions
under which:
octave
plot(xx,yy)
%% change the size of the plot window
%% and other plot options
....
close
plot(xx,yy)

results in the second plot(xx,yy) not
appearing the same as the first one.
(Using the current devel system.)

This is also very vague and right now I have
not been able to reproduce this behavior.

But, I suspect that close does not completely
reset all plot options.

If I can isolate a test case I will report it
under this bug #.

Michael Godfrey <godfrey>
Fri 07 Sep 2012 07:26:00 AM UTC, comment #2:

It's so obscure that perhaps priority can be decreased? - after all, nobody has noticed since at least 3.4.3.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 06 Sep 2012 09:03:11 PM UTC, comment #1:

Wow, that's an obscure bug.

I can confirm this with 3.6.2 on a Linux platform and it also affects the development code. You don't need to run all the demos. This shorter sequence reproduces the bug.

Interestingly, if you insert a "clf" between the demo and sombrero function call then it works. This seems to suggest that sombrero() is not clearing out the pre-existing figure.

Rik <rik5>
Project Member
Thu 06 Sep 2012 08:35:17 PM UTC, original submission:

Reproducible on Mandriva 2010.2,, Mageia 2 and Windows (MinGW, MSVC).
Also errors on Octave-3.6.2_MSVC/MinGW & Octave-3.6.1_MSVC

Works OK on MinGW Octave 3.4.3.

So somewhere in between 3.6.1 and 3.4.3 a regression happened.

With a "close" in between (before "sombrero") all goes well; fltk isn't affected either.

Philip Nienhuis <philipnienhuis>
Project Member

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #28098:  bad_attempt_for_defaults_2013may16.diff added by sebald (2kB - application/octet-stream)
file #26522:  sombrero-screenshot.png added by jwe (15kB - image/png)
file #26505:  changeset.patch added by bpabbott (2kB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jwe (Updated the item)
  • -unavailable- added by bpabbott (Updated the item)
  • -unavailable- added by sebald (Posted a comment)
  • -unavailable- added by godfrey (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by philipnienhuis (Submitted the item)
  •  

    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
    Fri 17 May 2013 04:02:45 AM UTCsebaldAttached File-=>Added bad_attempt_for_defaults_2013may16.diff, #28098
    Thu 16 May 2013 03:08:58 PM UTCbpabbottStatusFixed=>None
      Assigned tobpabbott=>None
    Mon 10 Sep 2012 04:20:12 PM UTCjweAttached File-=>Added sombrero-screenshot.png, #26522
      Open/ClosedClosed=>Open
    Mon 10 Sep 2012 03:26:23 PM UTCrik5Open/ClosedOpen=>Closed
    Sat 08 Sep 2012 06:57:59 PM UTCbpabbottStatusConfirmed=>Fixed
      Assigned toNone=>bpabbott
      Release3.6.3=>dev
    Sat 08 Sep 2012 01:36:30 AM UTCbpabbottAttached File-=>Added changeset.patch, #26505
    Sat 08 Sep 2012 12:28:40 AM UTCsebaldAttached File-=>Added octave-plotyyandappdata_2012sep07.patch, #26504
    Fri 07 Sep 2012 05:59:26 PM UTCrik5Severity3 - Normal=>2 - Minor
    Thu 06 Sep 2012 09:03:27 PM UTCrik5StatusNone=>Confirmed
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup