Opened 12 years ago

Closed 12 years ago

#10595 closed defect (fixed)

Vector constructor fails on empty list with no ring given

Reported by: Rob Beezer Owned by: jason, was
Priority: major Milestone: sage-4.7
Component: linear algebra Keywords:
Cc: Merged in: sage-4.7.alpha3
Authors: Rob Beezer, John Palmieri Reviewers: Dmitrii Pasechnik, John Palmieri, Rob Beezer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by John Palmieri)

sage: vector(QQ,[])
()

sage: vector([])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/sage/sage-4.6.1.rc1/devel/sage-main/<ipython console> in <module>()

/sage/sage-4.6.1.rc1/local/lib/python2.6/site-packages/sage/modules/free_module_element.so in sage.modules.free_module_element.vector (sage/modules/free_module_element.c:3229)()

/sage/sage-4.6.1.rc1/local/lib/python2.6/site-packages/sage/modules/free_module_element.so in sage.modules.free_module_element.prepare (sage/modules/free_module_element.c:3675)()

TypeError: unable to find a common ring for all elements

Default ring should be ZZ ideally.


Apply both

  • trac_10595-vector-constructor-empty-list.patch
  • 10595-referee.patch

Attachments (2)

trac_10595-vector-constructor-empty-list.patch (2.0 KB) - added by Rob Beezer 12 years ago.
10595-referee.patch (962 bytes) - added by John Palmieri 12 years ago.
apply on top of other patch

Download all attachments as: .zip

Change History (18)

Changed 12 years ago by Rob Beezer

comment:1 Changed 12 years ago by Rob Beezer

Authors: Rob Beezer
Status: newneeds_review

Patch depends on #10537, #10364.

comment:2 Changed 12 years ago by Dima Pasechnik

Status: needs_reviewpositive_review

comment:3 Changed 12 years ago by Jeroen Demeyer

Reviewers: Dmitrii Pasechnik

comment:4 Changed 12 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

This patch gives a massive number of doctest failures.

comment:5 Changed 12 years ago by Rob Beezer

The following existing (undocumented) behavior was broken by this patch, so needs to be reinstated or worked around. I thought this went through all a full test suite, but I guess not.

sage: z=ComplexField().an_element()
sage: z
1.00000000000000*I
sage: vector(z)
(0.000000000000000, 1.00000000000000)

comment:6 in reply to:  5 ; Changed 12 years ago by Dima Pasechnik

Replying to rbeezer:

The following existing (undocumented) behavior was broken by this patch, so needs to be reinstated or worked around. I thought this went through all a full test suite, but I guess not.

 sage: z=ComplexField().an_element()
 sage: z
 1.00000000000000*I
 sage: vector(z)
 (0.000000000000000, 1.00000000000000)

jeez, what an awful kludge...

comment:7 in reply to:  6 ; Changed 12 years ago by Rob Beezer

Replying to dimpase:

jeez, what an awful kludge...

Agreed. Worse that it was not documented, I think. And it causes about 80 doctest failures in one module, so there is probably no getting rid of it. So I'll figure out how it was happening in the first place, return the behavior, and document it.

Rob

comment:8 in reply to:  7 ; Changed 12 years ago by Dima Pasechnik

Replying to rbeezer:

Replying to dimpase:

jeez, what an awful kludge...

Agreed. Worse that it was not documented, I think. And it causes about 80 doctest failures in one module, so there is probably no getting rid of it. So I'll figure out how it was happening in the first place, return the behavior, and document it.

IMHO, if this "feature" is limited to complex numbers only, it better be eradicated. It should be easy to track down and fix the library code that uses it.

Dima

Rob

comment:9 in reply to:  8 ; Changed 12 years ago by Rob Beezer

Status: needs_workneeds_info

Replying to dimpase:

IMHO, if this "feature" is limited to complex numbers only, it better be eradicated. It should be easy to track down and fix the library code that uses it.

I agree - in principle. But I also have my hands full with some other changes in behavior. ;-)

I'll do some tests soon to gauge the magnitude of the chore and we can go from there.

Rob

comment:10 Changed 12 years ago by Rob Beezer

Status: needs_infoneeds_work

Here's the extent of converting complex numbers to vectors via the vector() constructor. Of course many of the failures are consequences of some earlier failure.

        sage -t  -force_lib pha2/devel/sage/doc/en/bordeaux_2008/elliptic_curves.rst # 3 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/crypto/mq/sbox.py # 5 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/crypto/mq/sr.py # 1 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/schemes/elliptic_curves/ell_egros.py # 8 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/schemes/elliptic_curves/period_lattice.py # 61 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/schemes/elliptic_curves/modular_parametrization.py # 8 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py # 30 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/schemes/elliptic_curves/constructor.py # 7 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/schemes/elliptic_curves/heegner.py # 81 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/schemes/elliptic_curves/ell_point.py # 16 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/schemes/generic/toric_divisor.py # 36 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/plot/colors.py # 1 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/algebras/quatalg/quaternion_algebra.py # 3 doctests failed
        sage -t  -force_lib pha2/devel/sage/sage/geometry/cone.py # 1 doctests failed

comment:11 in reply to:  9 ; Changed 12 years ago by Dima Pasechnik

Replying to rbeezer:

Replying to dimpase:

IMHO, if this "feature" is limited to complex numbers only, it better be eradicated. It should be easy to track down and fix the library code that uses it.

I agree - in principle. But I also have my hands full with some other changes in behavior. ;-)

I'll do some tests soon to gauge the magnitude of the chore and we can go from there.

how about the following:

make vector(blah) check whether blah.vector() can be called, and call it, if it is there.

add vector() member function to the corresponding complex number class.

Dima

Rob

comment:12 in reply to:  11 Changed 12 years ago by Rob Beezer

Replying to dimpase:

how about the following:

make vector(blah) check whether blah.vector() can be called, and call it, if it is there.

add vector() member function to the corresponding complex number class.

Yes, that's nice. Or even just call the member function every place it is needed, without adding more to the general vector constructor. That'd still require a fair bit of editing, but it would be mindless.

Rob

Changed 12 years ago by John Palmieri

Attachment: 10595-referee.patch added

apply on top of other patch

comment:13 Changed 12 years ago by John Palmieri

Status: needs_workneeds_review

Here's a patch which seems to fix the problem for me. I think the issue was this: some Sage objects (like complex numbers) can be converted to type "Sequence", and then those can be converted to vectors. But if those objects had no "len", then the patch would cause problems, so the "referee" patch tries to call "len", but ignores TypeErrors.

By the way, the "vector" function already checks to see if v._vector_() is defined, so another option would be to define a _vector_ method for complex numbers. This would fix many of the doctest errors, but not all of them, because some of them pass other objects (like generators) to "vector".

comment:14 Changed 12 years ago by John Palmieri

Description: modified (diff)

To the patchbot: Apply both

  • trac_10595-vector-constructor-empty-list.patch
  • 10595-referee.patch

comment:15 Changed 12 years ago by Rob Beezer

Authors: Rob BeezerRob Beezer, John Palmieri
Reviewers: Dmitrii PasechnikDmitrii Pasechnik, John Palmieri, Rob Beezer
Status: needs_reviewpositive_review

Referee patch does the job. Thanks, John. Dima and John have checked off on the original patch, and I'll check off on the referee patch.

Complex number behavior, and a bit more, will be documented on #10977.

comment:16 Changed 12 years ago by Jeroen Demeyer

Merged in: sage-4.7.alpha3
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.