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

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

Constructor quaternion algebra over number fields from ramification #37189

wants to merge 12 commits into from
+167 −5

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
@Eloitor Eloitor force-pushed the quatalg_from_ramification branch from d96b447 to b025292 Compare October 3, 2024 08:14
@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

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 left review comments

@S17A05 S17A05 S17A05 requested changes

Assignees
No one assigned
Labels
c: algebra p: major / 3 s: needs review 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
4 participants
@Eloitor @S17A05 @AurelPage @fchapoton