Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Constructor quaternion algebra over number fields from ramification #37189

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Eloitor
Copy link
Contributor

@Eloitor Eloitor commented Jan 29, 2024

This PR adds a new input format for the constructor of quaternion algebras.

It uses PARI alginit function to construct one form the ramification information.

Fixes #16948

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

@Eloitor
Copy link
Contributor Author

Eloitor commented Jan 29, 2024

I've seen this other PR #37173 that implements .ramified_places().

@Eloitor Eloitor force-pushed the quatalg_from_ramification branch 5 times, most recently from 8b4220c to 44fc263 Compare January 29, 2024 14:13
@S17A05
Copy link
Contributor

S17A05 commented Jan 29, 2024

I'll review this once the necessary checks have completed.

@Eloitor Eloitor changed the title Constructor quaternion algebra over number fields form ramification Constructor quaternion algebra over number fields from ramification Jan 29, 2024
src/sage/algebras/quatalg/quaternion_algebra.py Outdated Show resolved Hide resolved
src/sage/algebras/quatalg/quaternion_algebra.py Outdated Show resolved Hide resolved
src/sage/algebras/quatalg/quaternion_algebra.py Outdated Show resolved Hide resolved
src/sage/algebras/quatalg/quaternion_algebra.py Outdated Show resolved Hide resolved
src/sage/algebras/quatalg/quaternion_algebra.py Outdated Show resolved Hide resolved
src/sage/algebras/quatalg/quaternion_algebra.py Outdated Show resolved Hide resolved
src/sage/algebras/quatalg/quaternion_algebra.py Outdated Show resolved Hide resolved
src/sage/algebras/quatalg/quaternion_algebra.py Outdated Show resolved Hide resolved
src/sage/algebras/quatalg/quaternion_algebra.py Outdated Show resolved Hide resolved
@S17A05
Copy link
Contributor

S17A05 commented Jan 30, 2024

Let me know (e.g. by commenting here) once you think this is ready for another full review; in the meantime, Lint recognized some whitespaces in the blank line 302.

@Eloitor Eloitor force-pushed the quatalg_from_ramification branch 3 times, most recently from 882ba53 to 55809f3 Compare January 31, 2024 12:23
@Eloitor
Copy link
Contributor Author

Eloitor commented Jan 31, 2024

ready for review

@Eloitor Eloitor force-pushed the quatalg_from_ramification branch 3 times, most recently from 3be5ad7 to 0844c28 Compare January 31, 2024 12:33
@S17A05
Copy link
Contributor

S17A05 commented Jan 31, 2024

ready for review

Great, I'll review it again when I find the time for it; this will probably not be before Friday though.

@AurelPage
Copy link
Contributor

Did you check that the real embeddings are treated in the same order as in .real_embeddings()? This does not seem automatic as the real embeddings are computed from the roots of the defining polynomial, not from PARI, but you are simply forwarding the second argument to PARI's alginit. Maybe this should be checked, documented, and tested (by checking the signs of the embeddings of a,b).

Copy link

github-actions bot commented Jan 31, 2024

Documentation preview for this PR (built with commit b025292; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Copy link
Contributor

@S17A05 S17A05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. For the future, please don't force push big changes and instead put them into a new commit, otherwise it makes the review process a lot harder because everything has to be re-checked again.

Comment on lines 295 to 296
if category(K) is not NumberFields():
raise ValueError("quaternion algbera must be defined over a number field")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check category(QQ) is not NumberFields() returns True; you should instead place this check after handling the case of the rational field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sage: category(QQ)
Join of Category of number fields and Category of quotient fields and Category of metric spaces

Is it possible to use this to check if K is QQ or a number field?

Copy link
Contributor

@S17A05 S17A05 Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be possible, but even then I would advise against it: You need to handle the rational field separately anyways (I ran into the same issue in #37173), and up to the check is_RationalField(K) I don't see any code that doesn't work for either setting; hence it is probably easier to place the above category check in the else block after the treatment of the rational field case.

src/sage/algebras/quatalg/quaternion_algebra.py Outdated Show resolved Hide resolved
src/sage/algebras/quatalg/quaternion_algebra.py Outdated Show resolved Hide resolved
Comment on lines 316 to 318
A = pari(K).alginit([2, [fin_places_pari, [QQ(1/2)] * len(fin_places_pari)], arg2], maxord=0)
a = K(A.algsplittingfield().disc()[1])
b = K(A.algb())
Copy link
Contributor

@S17A05 S17A05 Feb 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately there is a major issue here that @AurelPage correctly detected: The order in which the real embeddings are computed in PARI is indeed not the same as the order of embeddings given by .real_embeddings().

Here is an example:
sage: K.<v> = NumberField(-3x^5 - 11x^4 - 4x^3 + 1)
sage: A = QuaternionAlgebra(K, [], [1/2, 0, 1/2])
Quaternion Algebra (-3
v^4 - 14v^3 - 15v^2 - 4v, -8) with base ring Number Field in v with defining polynomial -3x^5 - 11x^4 - 4x^3 + 1

Using the ramified_places code from #37173, one can check that this algebra ramifies at the two embeddings sending $v$ to $-0.738229069310693$ and to $0.467355922387407$. In K.real_embeddings() these are the second and the third embeddings, respectively, but the desired output should be ramified at the third embedding as well as the first embedding sending $v$ to $-3.25393276038648$.

This could be fixed by first precomputing the order in which PARI lists the embeddings, and then permuting the list of local Archimedean invariants accordingly. I am not sure if this is feasible, and maybe @AurelPage has a better idea on how to work around this discrepancy between Sage and PARI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@S17A05 It is not difficult to find the bijection. Consider a generator of the field $K$, and take an $\varepsilon$ that separates its real embeddings. Consider the sequences of real embeddings computed by sage and pari respectively, at precision smaller than $\varepsilon$. Sort them both using indirect sort (with the permutations) and compose the permutations in the right way.

However, this procedure should probably not be hard-coded in the middle of the quaternion algebras code, but should be put in a place that ensures it can be reused. Maybe it is already somewhere? I don't know where that should be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@S17A05 It is not difficult to find the bijection. Consider a generator of the field K, and take an ε that separates its real embeddings. Consider the sequences of real embeddings computed by sage and pari respectively, at precision smaller than ε. Sort them both using indirect sort (with the permutations) and compose the permutations in the right way.

However, this procedure should probably not be hard-coded in the middle of the quaternion algebras code, but should be put in a place that ensures it can be reused. Maybe it is already somewhere? I don't know where that should be.

I don't think this is implemented yet. I'm not sure if this is the most elegant solution, but I would suggest adding an optional boolean parameter (e.g. called pari_perm, set to False by default) to the .real_embeddings()-method of a number field such that, if the parameter is set to True, the method returns both the real embeddings and a permutation of the indices that correctly re-indexes the embeddings to be in the order they are computed in in PARI.

Comment on lines 196 to 211
sage: QuaternionAlgebra(QQ, [(2), (3)], [0])
Quaternion Algebra (-1, 3) with base ring Rational Field
sage: QuaternionAlgebra(QQ, [(2), (3)], [1/2])
Traceback (most recent call last):
...
ValueError: Quaternion algebra over the rationals must have an even number of ramified places
sage: K.<w> = NumberField(x^2-x-1)
sage: P = K.prime_above(2)
sage: Q = K.prime_above(3)
sage: A = QuaternionAlgebra(K, [P,Q], [0,0])
sage: A.discriminant()
Fractional ideal (6)
sage: A = QuaternionAlgebra(K, [P,Q], [1/2,0])
Traceback (most recent call last):
...
ValueError: Quaternion algebra over the rationals must have an even number of ramified places
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error warning in line 211 needs to be adapted, see the comment on line 314. Also, once #37173 has been reviewed and merged it would be nice to adapt the examples given here to show that the constructor gives an algebra with the desired ramification.

@S17A05
Copy link
Contributor

S17A05 commented Feb 13, 2024

The modifications of the constructor also seem to mess up some examples according to the details of the Build & Test / build failure, and on certain inputs there seems to be a SignalError: Segmentation fault thrown by A.algsplittingfield().disc(); we can look at these things once the code gives the correct mathematical result, though.

@S17A05
Copy link
Contributor

S17A05 commented Apr 14, 2024

@Eloitor I have found a fix for the communication between Sage and PARI - the main problem is that PARI optimizes the defining polynomial, which mixes the embeddings up a bit. By mimicking this optimization in Sage, one can easily compute the permutation there with a simple sorting (since the real embeddings in Sage are sorted already), though. If you want to, I can implement the solution if you give me the necessary access to your branch (I could also fix the merge conflicts and the issue raised in the other review comment).

@Eloitor
Copy link
Contributor Author

Eloitor commented Apr 14, 2024

This is great news!! Please go ahead. You should have access now :)

@S17A05
Copy link
Contributor

S17A05 commented Apr 14, 2024

Should be fixed; the new test examples aren't deterministic though (it can return different, but necessarily isomorphic algebras), so I've made sure that they are not actually tested right now - I'll make them into proper tests once #37173 has been merged. Also quickly mentioning myself here (@S17A05) so that the PR is a bit easier to find for me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Construct a quaternion algebra over a number field by specifying its ramification
3 participants