bugGNU Octave - Bugs: bug #37645, "set" function not...

 
 

bug #37645: "set" function not properly working with prop/val structure input.

Submitter:  Pantxo Diribarne <pantxo>
Submitted:  Sun 28 Oct 2012 03:03:55 PM UTC
   
 
Category:  Plotting Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Inaccurate Result
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

Wed 12 Dec 2012 11:17:24 PM UTC, comment #27: 
Ben Abbott <bpabbott>
Group Member
Wed 12 Dec 2012 09:37:42 PM UTC, comment #26: 

I actually tried your patch and there should be or instead of the and :


-  n = (n & strcmp (names, "activepositionproperty"));
+  n = (n | strcmp (names, "activepositionproperty"));


It seems to fix the test failure in copyobj.m.

Pantxo Diribarne <pantxo>
Group Member
Mon 10 Dec 2012 02:33:22 PM UTC, comment #25: 


>>>Comparing to the default values may not be reliable .. it is possible for property to have a default value and have its mode = "manual".

In that particular case there is no problem, the xxmode will be properly set if "manual" is not the default value. My approach may fail if only prop, or both prop and propmode are non-default. Your approach is thus the one we should prefer. The patch looks good.



 

Pantxo Diribarne <pantxo>
Group Member
Mon 10 Dec 2012 01:11:33 PM UTC, comment #24: 

Comparing to the default values may not be reliable .. it is possible for property to have a default value and have its mode = "manual".

The activepositionproperty can be added as below.  Are there any other properties that should be included?


diff --git a/scripts/plot/struct2hdl.m b/scripts/plot/struct2hdl.m
--- a/scripts/plot/struct2hdl.m
+++ b/scripts/plot/struct2hdl.m
@@ -92,6 +92,14 @@
     p = p(1:2, 1:(tst(end)-1));
   endif

+  ## Place the "*mode" properties as the end to avoid have the updaters
+  ## change the mode to "manual" when the value is "auto"
+  names = fieldnames (s.properties);
+  n = strncmp (cellfun (@fliplr, names, "uniformoutput", false), "edom", 4);
+  n = (n & strcmp (names, "activepositionproperty"));
+  names = [names(!n); names(n)];
+  s.properties = orderfields (s.properties, names);
+
   ## create object
   if (strcmp (s.type, "root"))
     h = 0;


Ben Abbott <bpabbott>
Group Member
Mon 10 Dec 2012 12:36:01 PM UTC, comment #23: 

The *mode properties are not the only ones that are bound to other ones. We should at least add activepositionproperty to the list of axes properties that should be set last.

Maybe setting only the non-default properties would be a better approach : a property XXX that has default value will also keep its  XXXmode default value = 'auto'.

Pantxo Diribarne <pantxo>
Group Member
Mon 10 Dec 2012 01:48:07 AM UTC, comment #22: 

I've attached a complete changeset

(file #27064)

Ben Abbott <bpabbott>
Group Member
Mon 10 Dec 2012 01:40:56 AM UTC, comment #21: 

The diff below (places the "*.mode" properties at the end) fixes the problem for me.


diff --git a/scripts/plot/struct2hdl.m b/scripts/plot/struct2hdl.m
--- a/scripts/plot/struct2hdl.m
+++ b/scripts/plot/struct2hdl.m
@@ -92,6 +92,13 @@
     p = p(1:2, 1:(tst(end)-1));
   endif

+  ## Place the "*mode" properties as the end to avoid have the updaters change the
+  ## mode to "manual" when the value is "auto"
+  names = fieldnames (s.properties);
+  n = strncmp (cellfun (@fliplr, names, "uniformoutput", false), "edom", 4);
+  names = [names(!n); names(n)];
+  s.properties = orderfields (s.properties, names);
+
   ## create object
   if (strcmp (s.type, "root"))
     h = 0;


If there are no objections, I can push a changeset.  I'm hesitating because there may be a property ending in "mode" which has a different meaning.  I don't think that is the case, but I thought I should ask first.

Ben Abbott <bpabbott>
Group Member
Mon 10 Dec 2012 01:20:40 AM UTC, comment #20: 

I've confirmed it is the properties that match "*mode".  The second axes has them as "manual", where the originals were "auto".

Perhaps the property names can just be reordered, placing those matching "*.mode" at the end?

Ben Abbott <bpabbott>
Group Member
Mon 10 Dec 2012 12:01:40 AM UTC, comment #19: 

The example below illustrates two problems.


h1 = figure ();
gca ();
set (gca (), "yscale", "log")
xlabel ("X");
ylabel ("Y")
s1 = hdl2struct (h1);
h2 = struct2hdl (s1);


Both the plot box aspect ratio and the yticklabels are different.

I suspect the problem is with order by which the properties for the copied figure are set.

Ben Abbott <bpabbott>
Group Member
Sun 09 Dec 2012 09:28:09 PM UTC, comment #18: 

After I pulled in this changeset just now, make check now fails for me on copyobj.

Mike Miller <mtmiller>
Group Member
Sun 09 Dec 2012 06:05:49 PM UTC, comment #17: 

Sorry for the delay.  I pushed the change to default.

http://hg.savannah.gnu.org/hgweb/octave/rev/1ac3c906990a

Ben Abbott <bpabbott>
Group Member
Tue 04 Dec 2012 07:30:43 PM UTC, comment #16: 

Why don't we put the change on the default branch.  There seems like there is a potential for unintended consequences which we don't want to have happen on stable.  On the development branch it is less of a concern and it can see some testing before release.

Rik <rik5>
Group administrator
Mon 19 Nov 2012 10:54:31 PM UTC, comment #15: 

Ok for the test. Thank you.

Pantxo Diribarne <pantxo>
Group Member
Mon 19 Nov 2012 10:33:06 PM UTC, comment #14: 

Updated changeset is attached.

Should this be applied to stable or default?

(file #26938)

Ben Abbott <bpabbott>
Group Member
Mon 19 Nov 2012 10:21:54 PM UTC, comment #13: 

Confirmed.  Ok to add the test below to the changeset?


%!test
%! markchanged = @(h, foobar, name) set (h, "userdata", [get(h,"userdata"); {name}]);
%! figure (1, "visible", "off")
%! clf ()
%! h = line ();
%! set (h, "userdata", {})
%! addlistener (h, "color", {markchanged, "color"})
%! addlistener (h, "linewidth", {markchanged, "linewidth"})
%! # linewidth first
%! props.linewidth = 2;
%! props.color = "r";
%! set (h, props);
%! assert (get (h, "userdata"), fieldnames (props))
%! clear props
%! clf ()
%! h = line ();
%! set (h, "userdata", {})
%! addlistener (h, "color", {markchanged, "color"})
%! addlistener (h, "linewidth", {markchanged, "linewidth"})
%! # color first
%! props.color = "r";
%! props.linewidth = 2;
%! set (h, props);
%! assert (get (h, "userdata"), fieldnames (props))
%! close (1)


Ben Abbott <bpabbott>
Group Member
Mon 19 Nov 2012 08:25:22 PM UTC, comment #12: 

Trying to track the revision that fixed the bug, I rebuilt octave from a fresh repo. Now I see the bug again (my repo was probably patched ... sorry). My tip is now :

changeset:   15613:126285fce876
tag:         tip
user:        Jordi Gutiérrez Hermoso <jordigh@octave.org>
date:        Thu Nov 15 13:07:24 2012 -0500
summary:     check for allowed struct field names in subsref and subsasgn


This script shows that properties are not set in the order they were stored:


function haschanged (h, p1, p2)
  printf ('%s has changed\n', p2)
endfunction

figure (1);
#linewidth first
prp.linewidth = 2;
prp.color = 'r';
h = line ();
addlistener (h, 'color', {@haschanged, 'color'})
addlistener (h, 'linewidth', {@haschanged, 'linewidth'})
set (h, prp);

#clean
close (1); clear prp

#color first
prp.color = 'r';
prp.linewidth = 2;
h = line ();
addlistener (h, 'color', {@haschanged, 'color'})
addlistener (h, 'linewidth', {@haschanged, 'linewidth'})
set (h, prp);


"close" property is always set first. Can someone confirm?

Pantxo Diribarne <pantxo>
Group Member
Mon 19 Nov 2012 05:10:50 PM UTC, comment #11: 

Attached is a bisect script I've used for bug #32818.

(file #26934)

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Mon 19 Nov 2012 05:02:16 PM UTC, comment #10: 

Automatically test wether the bug is fixed running octave "make check" is a bit dangerous as there are tests that may not pass but not be relevent. Can a simple octave script called with 'octave --eval mytest.m' throw an error that is detected as an error by bisect?

Pantxo Diribarne <pantxo>
Group Member
Mon 19 Nov 2012 04:04:29 PM UTC, comment #9: 


> I'm afraid I don't know how to determine which revision fixed the bug.


If you really care to find out,

http://www.selenic.com/mercurial/hg.1.html#bisect

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Mon 19 Nov 2012 03:44:30 PM UTC, comment #8: 

I'm afraid I don't know how to determine which revision fixed the bug.

Pantxo Diribarne <pantxo>
Group Member
Mon 19 Nov 2012 01:23:17 PM UTC, comment #7: 

Ok.  I'm changing the status to fixed.  Before closing, should a test be added?  Maybe a note to reference which changeset fixed this?  The latter is important so that we can include the test in the proper source file.


%!test
%! x = 0:0.1:10;
%! figure (1, "visible", "off");
%! clf ()
%! h(1) = plot (x, sin (x));
%! prop_in.position = get (gca (), "position");
%! prop_in.outerposition = get (gca (), "outerposition");
%! prop_in.activepositionproperty = get (gca (),"activepositionproperty");
%! ax(1) = gca ();
%! ax(2) = axes ();
%! h(2) = plot (x, cos (x), "r");
%! legend (ax(1), h, {"sin(x)", "cos(x)"}, "location", "northeast")
%! set (ax(2), prop_in);
%! set (ax(2), "color", "none")
%! prop_out.position = get (ax(2), "position");
%! prop_out.outerposition = get (ax(2), "outerposition");
%! prop_out.activepositionproperty = get (ax(2), "activepositionproperty");
%! assert (prop_out, prop_in)


Ben Abbott <bpabbott>
Group Member
Mon 19 Nov 2012 08:15:50 AM UTC, comment #6: 

Indeed, I don't the bug anymore neither in dev, and the change hasn't been pushed. There must have been a change upward that makes octave_map behave differently.

Pantxo Diribarne <pantxo>
Group Member
Fri 16 Nov 2012 02:58:04 PM UTC, comment #5: 

I'm not seeing the bug.  My tip is below.  Has this change been pushed, or corrected incidently by other changeset ?


changeset:   15612:8b04a7d67d8a
tag:         tip
parent:      15609:61d08b092a9d
user:        Carlo de Falco <cdf@users.sourceforge.net>
date:        Mon Nov 12 17:44:03 2012 +0100
summary:     Use texinfo syntax in default m-file template.


Ben Abbott <bpabbott>
Group Member
Tue 30 Oct 2012 01:40:53 PM UTC, comment #4: 

Here is a c++ beginner patch (set_PV.patch) that makes octave behave as ML : the property/value pairs are set in the order in which they where originally stored in the PV structure.

Can someone check it does indeed fix the inconsistancy?

(file #26856)

Pantxo Diribarne <pantxo>
Group Member
Mon 29 Oct 2012 11:18:26 AM UTC, comment #3: 

Well I have already reached my c++ limits :-) ... I dont't even understand when/how updtate_XXX functions are called.

ML does update the "activepositionproperty", depending on the order in which the PV structure fields where stored :
 

plot (1:10)
legend ('toto')
prp.position = get (gca, 'position');
prp.outerposition = get (gca, 'outerposition');
prp.activepositionproperty = get (gca,'activepositionproperty');
figure ();ax = axes (); set (ax, prp)


yields the same properties as the original,


...
prp.activepositionproperty = get (gca,'activepositionproperty');
prp.position = get (gca, 'position');
prp.outerposition = get (gca, 'outerposition');
figure ();ax = axes (); set (ax, prp)


yields "outerposition" as "activepositionproperty",


...
prp.activepositionproperty = get (gca,'activepositionproperty');
prp.outerposition = get (gca, 'outerposition');
prp.position = get (gca, 'position');
figure ();ax = axes (); set (ax, prp)


and this yieds to "position" as "activepositionproperty".

As far as I understand (thus not really far) I think avoiding the alphbetical sorting in graphics_object::set (const octave_map& m) would lead to the same behaviour in octave.

Pantxo Diribarne <pantxo>
Group Member
Mon 29 Oct 2012 09:14:52 AM UTC, comment #2: 

No, it is not a listener, it is:

    void update_outerposition (void)
    {
      set_activepositionproperty ("outerposition");
      sync_positions ();
    }

    void update_position (void)
    {
      set_activepositionproperty ("position");
      sync_positions ();
    }

in graphics.h.

The only way to fix this is by ensuring that activepositionproperty will be set last. Does ML handle this case correctly?

Konstantinos Poulios <logari81>
Sun 28 Oct 2012 06:46:10 PM UTC, comment #1: 

I tried to figure out the way prop/val structures where handled in graphics.cc, adding:
octave_stdout << name << std::endl; line 2048

 It looks like  the prop/val structure is stored in an octave_map after sorting the keys alphabetically. Am I right?

This would be the reason why "position" is always treated last, and forces "activepositionproperty" to become "position" (through a listener function??). I now have to see where the listener function is called and how It could be somehow disabled.

Pantxo Diribarne <pantxo>
Group Member
Sun 28 Oct 2012 03:03:55 PM UTC, original submission:  

Consider the following code that copies the position related properties from one axes to another:


figure (1); clf
plot (1:10)
legend ('toto')
prp.position = get (gca, 'position');
prp.outerposition = get (gca, 'outerposition');
prp.activepositionproperty = get (gca,'activepositionproperty');
disp (prp)

##set props using a PV structure
figure (2);
ax = axes ();
set (ax, prp);
prpout.position = get (ax, 'position');
prpout.outerposition = get (ax, 'outerposition');
prpout.activepositionproperty = get (ax, 'activepositionproperty');
disp (prpout)


The final position of the clone axes is not properly set: the "activepositionproperty" are not identical in prp and prpout.

Pantxo Diribarne <pantxo>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #27064:  changeset.patch added by bpabbott (843B - application/octet-stream)
file #26938:  changeset.patch added by bpabbott (2KiB - application/octet-stream)
file #26934:  bisect-32818.sh added by jordigh (449B - text/x-sh)
file #26856:  set_PV.patch added by pantxo (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by siko1056 (Updated the item)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by bpabbott (Posted a comment)
  • -email is unavailable- added by jordigh (Updated the item)
  • -email is unavailable- added by logari81 (Posted a comment)
  • -email is unavailable- added by pantxo (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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-09-28 siko1056 StatusPatch Submitted Fixed
    2012-12-10 bpabbott Attached File- Added changeset.patch, #27064
        StatusFixed Patch Submitted
    2012-12-09 bpabbott Open/ClosedOpen Closed
    2012-11-19 bpabbott Attached File- Added changeset.patch, #26938
    2012-11-19 jordigh Attached File- Added bisect-32818.sh, #26934
    2012-11-19 bpabbott StatusPatch Submitted Fixed
    2012-10-30 jordigh StatusNone Patch Submitted
    2012-10-30 pantxo Attached File- Added set_PV.patch, #26856

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code