bugGNU Octave - Bugs: bug #31536, demos for patch and isosurface fail

 
 

bug #31536: demos for patch and isosurface fail

Submitter:  Ben Abbott <bpabbott>
Submitted:  Wed 03 Nov 2010 02:39:32 PM UTC
   
 
Category:  None Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Ben Abbott Open/Closed:  * Closed
Release:  * dev Operating System:  * Mac OS
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Thu 04 Nov 2010 12:39:45 AM UTC, comment #5: 

Fixed in this changeset from Ben (http://hg.savannah.gnu.org/hgweb/octave/rev/0a8e554494c9).  Closing bug.

Rik <rik5>
Group administrator
Wed 03 Nov 2010 09:26:03 PM UTC, comment #4: 

Ok, I missed the significance of the second argument to the find function.. My objection is withdrawn

D.

David Bateman <dbateman>
Group Member
Wed 03 Nov 2010 09:07:00 PM UTC, comment #3: 

Oh no, I hope I'm not abusing the poor defenseless comma-separated lists.  :)

Actually, I might not understand the example well enough and I did look only briefly, but I think I avoid cs-lists altogether.


For hypothetical function call patch (...,"cdata", c1, "cdata", c2).  Diary from Octave session
args = {"cdata", [1,2,3], "cdata", [4,5,6]};
octave:2> args = { "cdata", [1,2,3], "cdata", [4,5,6] };
octave:3> class(args)
ans = cell
octave:4> eval1 = strcmpi (args, "cdata")
eval1 =

   1   0   1   0

octave:5> class eval1
ans = char
octave:6> eval2 = find(eval1, 1, "last")
eval2 =  3
octave:7> eval3 = eval2 + 1
eval3 =  4
octave:8> output_cdata = args {eval3}
output_cdata =

   4   5   6

octave:9> isvector (output_cdata )
ans =  1


What looks potentially weak to me is that if there are no matches then find returns the empty matrix and there is the reliance on '[] + 1 == []'

Rik <rik5>
Group administrator
Wed 03 Nov 2010 08:28:02 PM UTC, comment #2: 

Opps, I was trying the address the code like

patch(..., "cdata", c1, ...., "cdata", c2, ....)

where cdata should be taken to be c2. I didn't treat the case where its not at all defined. I like Rik proposal, but it (and Ben's proposal for tht matter) abuses cs-lists to get the right functionally. Consider a code snippet like

c = {[1,2,3],[4,5,6]}
c{[2,1]}
cdata = c{[2,1]}

The line c{[2,1]} returns a two element cs-list, whereas  cdata=c{[1,2]} only has a single argument on the LHS and so the second element of the cs-list is ignored. Its a bit of a nasty trick in my opinion to get the right behavior, but without making the code much more complex I don't see a way of getting the right behavior and avoiding indexing into null arrays. I'd say push a changeset based on Rik's suggestion

D.

David Bateman <dbateman>
Group Member
Wed 03 Nov 2010 07:31:15 PM UTC, comment #1: 

Using 'hg bisect' I find the problem was introduced in this changeset:

changeset:   11160:131d56b6d9a3
user:        David Bateman <dbateman@free.fr>
date:        Thu Oct 28 01:56:14 2010 +0200
summary:     Set cdata from facevertexcdata and visa versa for patch at all times

I've only briefly glanced at your patch.  We should definitely avoid indexing into null arrays, which is what your patch does.  However it does it for only 2 of the 4 occurrences of this code construct.  Would it be better to do it for all 4?  Also, the strcmp functions now work perfectly well on cell arrays so I think the code could eliminate the anonymous function call for legibility.  Sample below:

Original:
idx = find (cellfun (@(x) strcmpi (x, "facevertexcdata"), args))(end) + 1;
Proposed:
idx = find (cellfun (@(x) strcmpi (x, "facevertexcdata"), args), 1, "last") + 1;
My Suggestion:
idx = find (strcmpi (args, "facevertexcdata"), 1, "last") + 1;


Otherwise, the patch works for me.

Rik <rik5>
Group administrator
Wed 03 Nov 2010 02:39:32 PM UTC, original submission:  

The 3rd and 4th patch demos, and the isosurface demo, are failing.

This may be due to the recent changeset below (I'm just guessing).

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

I've attached a chageset which fixes the problem, but as I'm not sure this is the proper solution I won't push it until someone takes a closer look.

Ben Abbott <bpabbott>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dbateman (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by bpabbott (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-11-04 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2010-11-03 rik5 Item GroupNone Incorrect Result
        StatusNone Confirmed
        Carbon-Copy- Added -email is unavailable-
    2010-11-03 bpabbott Attached File- Added changeset.patch, #21913

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code