Opened 11 years ago

Closed 11 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:

Status badges

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)

trac_10601_QuaternionAlgebra_constructor.patch (4.4 KB) - added by spice 11 years ago.
Replaces previous patch

Download all attachments as: .zip

Change History (8)

comment:1 Changed 11 years ago by spice

  • Authors set to Simon Spicer
  • Keywords QuaternionAlgebra added
  • Status changed from new to needs_review

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

comment:2 Changed 11 years ago by rbeezer

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
    
  1. 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))
    
  1. It might be good practice to preface your new tests with a sentence saying they check Trac #10601.

comment:3 Changed 11 years ago by spice

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

comment:4 Changed 11 years ago by rbeezer

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

Changed 11 years ago by spice

Replaces previous patch

comment:5 follow-up: Changed 11 years ago by spice

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

  • 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 11 years ago by jdemeyer

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