Opened 10 years ago
Closed 10 years ago
#10601 closed defect (fixed)
QuaternionAlgebra constructor does not work for python int
Reported by: | lftabera | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7 |
Component: | algebra | Keywords: | QuaternionAlgebra |
Cc: | Merged in: | sage-4.7.alpha5 | |
Authors: | Simon Spicer | Reviewers: | Rob Beezer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
QuaternionAlgebra? constructor does not work with python int input. This is unfortunate for *.py scripts
Quaternion Algebra (1, 1) with base ring Rational Field sage: QuaternionAlgebra(1r,1) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) /home/luisfe/<ipython console> in <module>() /opt/SAGE/sage-devel/local/lib/python2.6/site-packages/sage/algebras/quatalg/quaternion_algebra.pyc in QuaternionAlgebra(arg0, arg1, arg2, names) 175 b = v[1] 176 else: --> 177 raise ValueError, "unknown input" 178 179 # QuaternionAlgebra(K, a, b) ValueError: unknown input sage: QuaternionAlgebra(1,1r) Quaternion Algebra (1, 1) with base ring Rational Field sage: QuaternionAlgebra(1r,1r) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) /home/luisfe/<ipython console> in <module>() /opt/SAGE/sage-devel/local/lib/python2.6/site-packages/sage/algebras/quatalg/quaternion_algebra.pyc in QuaternionAlgebra(arg0, arg1, arg2, names) 175 b = v[1] 176 else: --> 177 raise ValueError, "unknown input" 178 179 # QuaternionAlgebra(K, a, b) ValueError: unknown input
The problem seems to be the first numeric parameter. This will be easy to fix.
Attachments (1)
Change History (8)
comment:1 Changed 10 years ago by
- Keywords QuaternionAlgebra added
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
OK, now I see that the following has always been possible:
sage: T=QuaternionAlgebra(RR(3), RR(3)) sage: T Quaternion Algebra (3.00000000000000, 3.00000000000000) with base ring Real Field with 53 bits of precision
This looks good - some suggestions follow. Let me know, and then I'll run tests.
- Maybe
is_RingElement
would be an improved test, as it would catch some things sooner and give a better error message?sage: a=PermutationGroupElement([1,2,3]) sage: QuaternionAlgebra(a, a) <snip> AttributeError: 'SymmetricGroup_with_category' object has no attribute 'fraction_field' sage: is_Element(a) <snip> True sage: is_RingElement(a) False
- I think that the error message could be more informative. Probably be good to have a doctest that raises this error (you could use the permutation example above).
ValueError('input is not an element of a ring: {0}'.format(a))
- It might be good practice to preface your new tests with a sentence saying they check Trac #10601.
comment:3 Changed 10 years ago by
Thanks for the pointers. I've updated the patch as per your three suggestions.
comment:4 Changed 10 years ago by
Simon,
Looks good and passes all long tests. Can you make just one more edit?
# The following tests address the issues rased in trac ticket 10601
can go right up in the text preceding, that seems to be the custom - ie as part of the visible documentation (and "raised" needs a letter). If you can do that, I'll just peek at the patch and we'll be done. Thanks for your work on this.
Rob
comment:5 follow-up: ↓ 6 Changed 10 years ago by
Done. Changed to:
Python ints, longs and floats may be passed to the ``QuaternionAlgebra(a, b)`` constructor, as may all pairs of nonzero elements of a ring not of characteristic 2. The following tests address the issues raised in trac ticket 10601:: sage: QuaternionAlgebra(1r,1) Quaternion Algebra (1, 1) with base ring Rational Field sage: QuaternionAlgebra(1,1.0r) Quaternion Algebra (1.00000000000000, 1.00000000000000) with base ring Real Field with 53 bits of precisi\ on
etc.
Apologies, I'm still a bit green when it comes to SageDev?; still trying to get used to all the coding conventions :-)
comment:6 in reply to: ↑ 5 Changed 10 years ago by
- Reviewers set to Rob Beezer
- Status changed from needs_review to positive_review
Replying to spice:
Apologies, I'm still a bit green when it comes to SageDev?; still trying to get used to all the coding conventions :-)
No problem - there's a lot to keep track of, and that's what reviews are for. (And I could have been more explicit about where to put the Trac number.)
This looks real good. Positive review
comment:7 Changed 10 years ago by
- Merged in set to sage-4.7.alpha5
- Resolution set to fixed
- Status changed from positive_review to closed
Fixed the QuaternionAlgebra? constructor to accept Python ints, longs and floats as input. My code's a bit inelegant, so review/rewrite is welcome.