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

 
 

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

Submitted by:  Pantxo Diribarne <pantxo>
Submitted on:  Sun 28 Oct 2012 03:03:55 PM UTC  
 
Category: PlottingSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Inaccurate Result
Status: Patch SubmittedAssigned to: None
Originator Name: Open/Closed: Closed
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)

Wed 12 Dec 2012 11:17:24 PM UTC, comment #27:

Thanks

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

Ben Abbott <bpabbott>
Project 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 :

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

Pantxo Diribarne <pantxo>
Project 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>
Project 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?

Ben Abbott <bpabbott>
Project 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>
Project Member
Mon 10 Dec 2012 01:48:07 AM UTC, comment #22:

I've attached a complete changeset

(file #27064)

Ben Abbott <bpabbott>
Project 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.

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>
Project 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>
Project Member
Mon 10 Dec 2012 12:01:40 AM UTC, comment #19:

The example below illustrates two problems.

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>
Project 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>
Project 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>
Project 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>
Project Administrator
Mon 19 Nov 2012 10:54:31 PM UTC, comment #15:

Ok for the test. Thank you.

Pantxo Diribarne <pantxo>
Project 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>
Project Member
Mon 19 Nov 2012 10:21:54 PM UTC, comment #13:

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

Ben Abbott <bpabbott>
Project 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:

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

Pantxo Diribarne <pantxo>
Project 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>
Project Administrator
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>
Project 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>
Project Administrator
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>
Project 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.

Ben Abbott <bpabbott>
Project 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>
Project 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 ?

Ben Abbott <bpabbott>
Project 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>
Project 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 :

yields the same properties as the original,

yields "outerposition" as "activepositionproperty",

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>
Project 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>
Project Member
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>
Project 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:

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

Pantxo Diribarne <pantxo>
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 #27064:  changeset.patch added by bpabbott (843B - application/octet-stream)
file #26938:  changeset.patch added by bpabbott (2kB - application/octet-stream)
file #26934:  bisect-32818.sh added by jordigh (449B - text/x-sh)
file #26856:  set_PV.patch added by pantxo (1kB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by bpabbott (Posted a comment)
  • -unavailable- added by jordigh (Updated the item)
  • -unavailable- added by logari81 (Posted a comment)
  • -unavailable- added by pantxo (Submitted the item)
  •  

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

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 10 Dec 2012 01:48:07 AM UTCbpabbottAttached File-=>Added changeset.patch, #27064
      StatusFixed=>Patch Submitted
    Sun 09 Dec 2012 06:05:49 PM UTCbpabbottOpen/ClosedOpen=>Closed
    Mon 19 Nov 2012 10:33:06 PM UTCbpabbottAttached File-=>Added changeset.patch, #26938
    Mon 19 Nov 2012 05:10:50 PM UTCjordighAttached File-=>Added bisect-32818.sh, #26934
    Mon 19 Nov 2012 01:23:17 PM UTCbpabbottStatusPatch Submitted=>Fixed
    Tue 30 Oct 2012 02:50:00 PM UTCjordighStatusNone=>Patch Submitted
    Tue 30 Oct 2012 01:40:53 PM UTCpantxoAttached File-=>Added set_PV.patch, #26856

    Back to the top


    Powered by Savane 3.1-cleanup