Opened 9 years ago

Closed 8 years ago

#10595 closed defect (fixed)

Vector constructor fails on empty list with no ring given

Reported by: rbeezer 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:

Description (last modified by jhpalmieri)

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 rbeezer 9 years ago.
10595-referee.patch (962 bytes) - added by jhpalmieri 9 years ago.
apply on top of other patch

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by rbeezer

  • Authors set to Rob Beezer
  • Status changed from new to needs_review

Patch depends on #10537, #10364.

comment:2 Changed 9 years ago by dimpase

  • Status changed from needs_review to positive_review

comment:3 Changed 9 years ago by jdemeyer

  • Reviewers set to Dmitrii Pasechnik

comment:4 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This patch gives a massive number of doctest failures.

comment:5 follow-up: Changed 9 years ago by 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)

comment:6 in reply to: ↑ 5 ; follow-up: Changed 9 years ago by dimpase

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 ; follow-up: Changed 9 years ago by 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.

Rob

comment:8 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by dimpase

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 ; follow-up: Changed 9 years ago by rbeezer

  • Status changed from needs_work to needs_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 9 years ago by rbeezer

  • Status changed from needs_info to needs_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 ; follow-up: Changed 9 years ago by dimpase

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 9 years ago by rbeezer

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 9 years ago by jhpalmieri

apply on top of other patch

comment:13 Changed 9 years ago by jhpalmieri

  • Status changed from needs_work to needs_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 9 years ago by jhpalmieri

  • Description modified (diff)

To the patchbot: Apply both

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

comment:15 Changed 9 years ago by rbeezer

  • Authors changed from Rob Beezer to Rob Beezer, John Palmieri
  • Reviewers changed from Dmitrii Pasechnik to Dmitrii Pasechnik, John Palmieri, Rob Beezer
  • Status changed from needs_review to positive_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 8 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.