#13654: A normalized quadratic form for quaternion algebras.
---------------------------------------------+------------------------------
Reporter: schisholm | Owner: AlexGhitza
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-5.5
Component: algebra | Resolution:
Keywords: Normalized Quadratic Form | Work issues:
Report Upstream: N/A | Reviewers: tkluck
Authors: Sarah Chisholm | Merged in:
Dependencies: | Stopgaps:
---------------------------------------------+------------------------------
Changes (by tkluck):
* status: needs_review => needs_work
* reviewer: => tkluck
* type: PLEASE CHANGE => enhancement
Comment:
Thanks for contributing! I've had a look at your code. It looks like a
good implementation of the algorithm in the article. I don't know much
about quadratic forms, so I can't really say much about the math.
My most important review point is that your doctests don't cover all your
code paths. The examples you give already have a diagonal inner product
matrix, so all but the first step are not being tested.
In fact, the other code paths contain `1/2 in R` which I'm not sure will
work as you seem to expect. I think it will just throw a
`ZeroDivisionError` if 2 is not a unit. You could replace it by
`R(2).is_unit()`.
I'll set this ticket's status to needs_work.
Here's a few additional suggestions to make your code more concise by
using a few Python gimmicks:
* Instead of
{{{
E = self.basis()
f = []
for i in range(len(E)):
f.append(E[i])
}}}
for copying a list, you can write
{{{
f = self.basis()[:]
}}}
* You could implement a function `T(x,y)` because you use that expression
a lot. It makes the code a lot more readable.
* Usually, you shouldn't need to have loops like
{{{
for i in xrange(len(f)):
# do something with f[i]
}}}
because you can write
{{{
for f_i in f:
# do something with f_i
}}}
* One hard part is the sorting prescribed by
> Otherwise, let (i, j) with 1 ≤ i ≤ j ≤ n be such that ord T(e_i, e_j) is
minimal, taking i = j if possible and if not taking i minimal.
I've found a way to implement that using the builtin `sort`. Namely, this
is equivalent to sorting the tuples `(valuation, int(i != j), i)`
lexicographically.
I've attached a patch including these comments.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/13654#comment:2>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
