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.

Sign up for GitHub

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

Jump to bottom

Constructor quaternion algebra over number fields from ramification #37189

Merged
merged 15 commits into from
Mar 22, 2025
Merged

Constructor quaternion algebra over number fields from ramification #37189

merged 15 commits into from
Mar 22, 2025

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
Member

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
S17A05
S17A05 reviewed Jan 29, 2024
View reviewed changes
@Eloitor Eloitor force-pushed the quatalg_from_ramification branch 5 times, most recently from 5be2ea6 to d96f42e Compare January 30, 2024 11:52
@S17A05 S17A05 self-requested a review January 30, 2024 14:47
@S17A05
Copy link
Member

S17A05 commented Jan 30, 2024 •
edited
Loading

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
Member

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.

@Eloitor Eloitor force-pushed the quatalg_from_ramification branch from 0844c28 to 2657bae Compare January 31, 2024 15:40
@S17A05
Copy link
Member

S17A05 commented Oct 15, 2024

The errors in the tests seemed to be outdated, so I updated the branch to re-trigger the tests

@S17A05
Copy link
Member

S17A05 commented Oct 16, 2024

The errors in the tests seemed to be outdated, so I updated the branch to re-trigger the tests

Whoops, turns out I was looking at the wrong thing - the new commit should fix the errors.

@S17A05
Copy link
Member

S17A05 commented Oct 16, 2024

The timeout in src/sage/rings/tests.py seems unrelated, but should probably be investigated either way.

@S17A05
Copy link
Member

S17A05 commented Feb 11, 2025

Since #37173 should be on its way to get merged, I am putting this PR to s: needs work for a moment - once the former PR is merged, I'll add the missing tests for error messages and will modify the examples to properly check that the construction works correctly.

S17A05 and others added 2 commits February 21, 2025 23:16
@S17A05
Merge branch 'develop' into quatalg_from_ramification
3ba69c5
@S17A05
Added & adapted tests
5458599 
- Added missing tests for possible errors
- Integrated `.ramified_places()` to check that new construction works correctly
- Modified input checks to properly reject bad inputs
- Added author entry for prior work on sagemath#37173, sagemath#37644 and sagemath#37675
@S17A05
Copy link
Member

S17A05 commented Feb 21, 2025

I've added the new tests/examples, including the use of .ramified_places() to check that the construction works correctly. I've also made some small changes to the input checks to make sure that they reject bad inputs properly. Hopefully everything is covered now :)

@fchapoton
Copy link
Contributor

failing doctest

@S17A05
Copy link
Member

S17A05 commented Feb 22, 2025

failing doctest

Whoops, should be fixed now

AurelPage
AurelPage approved these changes Mar 12, 2025
View reviewed changes
Copy link
Contributor

@AurelPage AurelPage left a comment

Choose a reason for hiding this comment

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

Apart from a tiny nitpick, LGTM.

src/sage/algebras/quatalg/quaternion_algebra.py Outdated
raise ValueError("quaternion algebra over a number field must have an even number of ramified places")

# We want to compute the correct quaternion algebra over K with PARI
# As PARI optimizes the polynomial used to define the number field, we need to precompute
Copy link
Contributor

Choose a reason for hiding this comment

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

About the comment: PARI does not optimise the polynomial (by default, it does with flag=2), but it forces a defining polynomial that is monic and integral. If it is not, the polynomial gets changed.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation! I adjusted the comment to be more accurate

AurelPage
AurelPage reviewed Mar 18, 2025
View reviewed changes
src/sage/algebras/quatalg/quaternion_algebra.py Outdated

# Compute the correct quaternion algebra over L in PARI
A = L.__pari__().alginit([2, [fin_places_pari, [QQ((1,2))] * len(fin_places_pari)],
inv_arch_pari], maxord=0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that the new fails are due to a rename of the flag maxord to flag in pari's alginit which must have made its way into cypari2.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pointing this out! Hopefully everything should work now

@vbraun vbraun merged commit b9186e0 into sagemath:develop Mar 22, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fchapoton fchapoton fchapoton left review comments

@AurelPage AurelPage AurelPage approved these changes

@S17A05 S17A05 S17A05 requested changes

Assignees
No one assigned
Labels
c: algebra p: major / 3 t: enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Construct a quaternion algebra over a number field by specifying its ramification
5 participants
@Eloitor @S17A05 @AurelPage @fchapoton @vbraun