Opened 9 years ago
Closed 9 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 )
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 9 years ago by
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Status changed from needs_review to positive_review
comment:3 Changed 9 years ago by
- Reviewers set to Dmitrii Pasechnik
comment:4 Changed 9 years ago by
- Status changed from positive_review to needs_work
This patch gives a massive number of doctest failures.
comment:5 follow-up: ↓ 6 Changed 9 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 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 9 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 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 9 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 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 9 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 in reply to: ↑ 8 ; follow-up: ↓ 11 Changed 9 years ago by
- 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
- 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: ↓ 12 Changed 9 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 in reply to: ↑ 11 Changed 9 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 9 years ago by
- 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
- 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
- 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 9 years ago by
- Merged in set to sage-4.7.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
Patch depends on #10537, #10364.