Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#2667 closed defect (fixed)

[with patch, positive review] transform.pyx calls matrix() with an RDF vector inside of a list instead of a flat list.

Reported by: jason Owned by: was
Priority: major Milestone: sage-3.0
Component: graphics Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

When applying the patch for a overhauled matrix() function at #2651, I get doctest failures from sage/plot/plot3d/transform.pyx related to calling matrix() with a list of rows, but specifying a number of rows that conflicts.

You can see these failures by applying the patch and running sage -t -long on devel/sage/sage/plot/plot3d/shapes2.py (and the same failures make a whole bunch of other doctests fail too).

For transform.pyx, the call to matrix on line 44 appears to flatten the trans argument (i.e., list(trans)), but many times what is actually passed to Sage is a list containing a single RDF vector instead.

Attachments (3)

transform-trac2667.patch (1.5 KB) - added by rhinton 13 years ago.
2667.patch (690 bytes) - added by mhansen 13 years ago.
transform-trac2667.2.patch (690 bytes) - added by rhinton 13 years ago.
Good catch, mhansen. Apply this patch instead of the previous patch.

Download all attachments as: .zip

Change History (9)

Changed 13 years ago by rhinton

comment:1 Changed 13 years ago by rhinton

  • Summary changed from transform.pyx calls matrix() with an RDF vector inside of a list instead of a flat list. to [with patch, needs review] transform.pyx calls matrix() with an RDF vector inside of a list instead of a flat list.

I discussed this solution with robertwb and cwitty. I added a __list__() method to RealDoubleVectorSpaceElement to allow its elements to be converted to a list. This makes the hand-off between Transform and matrix() work (see transform.pyx line 44).

However, the vector was still coming into the Transform object wrapped in a one-element list. The problem was that Graphics3d.translate() allows a variable number of arguments for convenience. Before, if the first argument was a list or a tuple (note NOT a vector), this sequence was passed directly to self.transform(). As suggested by robertwb, replacing the code

        if isinstance(x[0], (tuple, list)):
            x = x[0]

with

        if len(x)==1:
            x = x[0]

works since a sequence is the only acceptable one-argument input in this case. This solution also avoids having to check types.

Note that changing the isinstance() call in the scale() method just below DOES NOT work. I didn't take the time to figure out why; everything seems to be working now. (The special-case code around line 627 and the fact that scale() is meaningful with only one input argument probably have something to do with it.)

comment:2 Changed 13 years ago by mhansen

I don't think list does anything. See #2626 .

Changed 13 years ago by mhansen

comment:3 Changed 13 years ago by mhansen

  • Summary changed from [with patch, needs review] transform.pyx calls matrix() with an RDF vector inside of a list instead of a flat list. to [with patch, positive review] transform.pyx calls matrix() with an RDF vector inside of a list instead of a flat list.

I've removed list. The patch applies, and sage -t -long on devel/sage/sage/plot/plot3d/shapes2.py passes when #2651 is applied.

comment:4 Changed 13 years ago by jason

apply just 2667.patch; rhinton and mhansen (and maybe robertwb and cwitty?) should all probably get credit.

comment:5 Changed 13 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 3.0.alpha0

Changed 13 years ago by rhinton

Good catch, mhansen. Apply this patch instead of the previous patch.

comment:6 Changed 13 years ago by rhinton

Sorry, my page must not have refreshed. Ignore my last patch and use mhansen's instead.

Note: See TracTickets for help on using tickets.