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: |
Description (last modified by )
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)
Change History (18)
Changed 12 years ago by
Attachment: | trac_10595-vector-constructor-empty-list.patch added |
---|
comment:1 Changed 12 years ago by
Authors: | → Rob Beezer |
---|---|
Status: | new → needs_review |
comment:2 Changed 12 years ago by
Status: | needs_review → positive_review |
---|
comment:3 Changed 12 years ago by
Reviewers: | → Dmitrii Pasechnik |
---|
comment:4 Changed 12 years ago by
Status: | positive_review → needs_work |
---|
This patch gives a massive number of doctest failures.
comment:5 follow-up: 6 Changed 12 years ago by
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 follow-up: 7 Changed 12 years ago by
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 follow-up: 8 Changed 12 years ago by
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 follow-up: 9 Changed 12 years ago by
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 follow-up: 11 Changed 12 years ago by
Status: | needs_work → 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 12 years ago by
Status: | needs_info → 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 follow-up: 12 Changed 12 years ago by
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 Changed 12 years ago by
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
comment:13 Changed 12 years ago by
Status: | needs_work → 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 12 years ago by
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
Authors: | Rob Beezer → Rob Beezer, John Palmieri |
---|---|
Reviewers: | Dmitrii Pasechnik → Dmitrii Pasechnik, John Palmieri, Rob Beezer |
Status: | needs_review → 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 12 years ago by
Merged in: | → sage-4.7.alpha3 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Patch depends on #10537, #10364.