QuaternionAlgebra constructor does not work for python int

[lftabera]

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.

Component: algebra

Keywords: QuaternionAlgebra

Author: Simon Spicer

Reviewer: Rob Beezer

Merged: sage-4.7.alpha5

Issue created by migration from https://trac.sagemath.org/ticket/10601

[haikona]

Author: Simon Spicer

[haikona]

comment:1

Fixed the QuaternionAlgebra constructor to accept Python ints, longs and floats as input. My code's a bit inelegant, so review/rewrite is welcome.

[haikona]

Changed keywords from none to QuaternionAlgebra

[rbeezer]

comment:2

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.

1. 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

2. 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))

3. It might be good practice to preface your new tests with a sentence saying they check Trac QuaternionAlgebra constructor does not work for python int #10601.

[haikona]

comment:3

Thanks for the pointers. I've updated the patch as per your three suggestions.

[rbeezer]

comment:4

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

[haikona]

Replaces previous patch

[haikona]

comment:5

Attachment: trac_10601_QuaternionAlgebra_constructor.patch.gz

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 :-)

[rbeezer]

comment:6

Replying to @haikona:

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

[rbeezer]

Reviewer: Rob Beezer

[jdemeyer]

Merged: sage-4.7.alpha5