Open mmasdeu opened 9 years ago
The code passes all doctests and runs in reasonable time. I think it is ready for review.
Just a remark: this functionality is in PARI now, so you could just try to wrap PARI.
Please use python3 raise syntax: raise Error('msg')
see patchbot report in plugin raise_statement
for the lines with a bad raise syntax
Changed branch from u/mmasdeu/16948-quatalg-from-ramification to public/ticket/16948
Branch pushed to git repo; I updated commit sha1. New commits:
b29e4f8 | trac #16948 some details in code and documentation |
New commits:
e5c0916 | Added missing docstrings in number_field.py so that patchbot is happy. |
Changed branch from public/ticket/16948 to u/mmasdeu/16948
This no longer merges with sage 7.2 (according to trac).
Changed branch from u/mmasdeu/16948 to public/16948
Branch pushed to git repo; I updated commit sha1. New commits:
cdae032 | Merge branch 'public/16948' in 7.4.b5 |
A small remark regarding the ValueError in line 636: the exponents in the discriminant factorization should be equal to 1, and not just odd.
By the way, is there a reason why the input is an ideal of F instead of a list of prime ideals of F? If I want to construct a quaternion algebra with a prescribed ramification set of finite primes, I'd have to use their product as input, and then the constructor would have to factor it back (see line 634).
Branch pushed to git repo; I updated commit sha1. New commits:
5284a37 | Merge branch 'public/16948' in 7.6.b4 |
Branch pushed to git repo; I updated commit sha1. New commits:
73272e3 | trac 16948 fixing bad merge |
Branch pushed to git repo; I updated commit sha1. New commits:
355c58d | trac 16498 some details in weak_approximation |
Thanks for the comments! Please take a look at the changes I have introduced.
Replying to @nsirolli:
A small remark regarding the ValueError in line 636: the exponents in the discriminant factorization should be equal to 1, and not just odd.
By the way, is there a reason why the input is an ideal of F instead of a list of prime ideals of F? If I want to construct a quaternion algebra with a prescribed ramification set of finite primes, I'd have to use their product as input, and then the constructor would have to factor it back (see line 634).
New commits:
fcf1a69 | Trac 16948: adopted changes proposed by nsirolli. |
Changed branch from public/16948 to u/mmasdeu/16948-review
Thanks for accepting my proposal.
Now the new input format, QuaternionAlgebra(K, "list of primes", S) is quite different from the one you are extending to number fields other than QQ, QuaternionAlgebra(D). I feel this controversy should be avoided. The problem could be addressed by letting this new constructor accept both a discriminant and a list of primes as input. A more radical solution would be to change the classical QuaternionAlgebra(D) to take a list of primes as input, but I guess that it could cause trouble.
Besides this, here are some remarks about the changes introduced in this commit:
Line 245 of quaternion_algebra.py should be fixed, since it does not reflect the new input format.
The documentation needs to take account of these changes; see lines 92-94, 605-607, 625 of quaternion_algebra.py and line 1835 of number_fields.py
Line 589 of quaternion_algebra.py and lines 1804, 1815, 1817 of number_fields.py: shouldn't the name of the list of prime ideales be L or P instead of I?
needs rebase, does not apply
There is a need for the constructor QuaternionAlgebra to take as input a number field F and a ramification (specified by an ideal D in F and a set of real places S of F) and which constructs a quaternion algebra over F with ramified precisely at the primes dividing D and at the places in S.
We have code to add here, which is now being tested and documented.
CC: @tornaria
Component: number theory
Keywords: quaternion algebra, ramification
Author: Marc Masdeu, Xavier Guitart
Branch/Commit: u/mmasdeu/16948-review @
fcf1a69
Issue created by migration from https://trac.sagemath.org/ticket/16948