Tests to see if ideal in quaternion algebra is primitive (cyclic) #37112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Your docstrings are not formatted properly. I have reformatted your
|
|
Also if you look at the tests ^, and in particular "Lint / lint (pull_request)", it fails for several formatting reasons. Please also edit them :) It's simple things like "blank line contains whitespace" and "trailing whitespace". PS: This indicates your editor is messed up - it should remove these for you. |
yyyyx4
reviewed
Jan 22, 2024
| O_basis = self.left_order().basis_matrix() | ||
|
|
||
| # Write I in the basis of its left order via rref | ||
| M = block_matrix(1,2,[O_basis.transpose(),I_basis.transpose()]).rref()[:,4:] |
There was a problem hiding this comment.
I believe this is just M = O_basis.solve_left(I_basis)?
|
|
||
| # Write I in the basis of its left order via rref | ||
| M = block_matrix(1,2,[O_basis.transpose(),I_basis.transpose()]).rref()[:,4:] | ||
| g = Integer(gcd((gcd(M_row) for M_row in M))) |
There was a problem hiding this comment.
Shorter way: g = Integer(gcd(M.list())).
| g = Integer(gcd((gcd(M_row) for M_row in M))) | ||
|
|
||
| # If g is 1 then the ideal is primitive | ||
| if g == 1: |
There was a problem hiding this comment.
The test g.is_one() is significantly faster than g == 1, since the latter goes through the type coercion system to find a common parent type to do the comparison in.
Comment on lines
3046
to
3052
| if self.__left_order is not None: | ||
| return all([b in self.left_order() for b in self.basis()]) | ||
| elif self.__right_order is not None: | ||
| return all([b in self.right_order() for b in self.basis()]) | ||
| else: | ||
| self_square = self**2 | ||
| return all([b in self for b in self_square.basis()]) |
There was a problem hiding this comment.
Each of these tests should be equivalent to one of the form A.free_module() <= B.free_module(). (See also #37113.)
|
|
||
| """ | ||
| _,g = self.primitive_decomposition() | ||
| return 1 == g |
There was a problem hiding this comment.
Same here, using .is_one() is faster.
In this small commit I inserted all the improvements pointed out previously by @yyyyx4 (many thanks!)
yyyyx4
reviewed
Jan 29, 2024
Comment on lines
3113
to
3115
| TESTS:: | ||
|
|
||
| Checks on random crafted ideals that they decompose as expected. |
There was a problem hiding this comment.
One more linter failure:
Suggested change
| TESTS:: | |
| Checks on random crafted ideals that they decompose as expected. | |
| TESTS: | |
| Checks on random crafted ideals that they decompose as expected:: |
(The :: syntax only indicates that an indented block is about to follow, so here it should be moved down as indicated instead of sticking to the TESTS: header.)
yyyyx4
reviewed
Jan 29, 2024
| @@ -56,7 +56,7 @@ | |||
| from sage.structure.category_object import normalize_names | |||
| from sage.structure.parent import Parent | |||
| from sage.matrix.matrix_space import MatrixSpace | |||
| from sage.matrix.constructor import diagonal_matrix, matrix | |||
| from sage.matrix.constructor import diagonal_matrix, matrix, block_matrix | |||
There was a problem hiding this comment.
The import of block_matrix is now unused and can be removed.
|
Documentation preview for this PR (built with commit 09d352e; changes) is ready! 🎉 |
yyyyx4
approved these changes
Feb 1, 2024
|
Looks good to me, thanks! |
3 tasks
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Apr 18, 2024
I have implemented the `automatic size labeler`, which now assigns labels to pull requests based on the number of lines changed **Minimal** Typically involves very small changes, bug fixes, or updates that require only a few lines of code, often less than 50. sagemath#37208 sagemath#37146 sagemath#37043 **Small** Involves more substantial changes than minimal, potentially adding new features or making modifications to existing ones. The range is usually between 50 to 100 lines of code. sagemath#37152 sagemath#37132 **Moderate** Represents a significant portion of the codebase being modified, such as adding new features, refactoring, or making extensive changes to existing functionalities. This might involve between 100 to 300 lines of code. sagemath#36919 sagemath#37112 **Large** Involves substantial and complex changes across various parts of the codebase. This could include major architectural changes, the introduction of new modules, or a significant overhaul of existing features, often exceeding 300 lines of code. sagemath#37125 sagemath#36977 sagemath#36972 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. Fixes: sagemath#37254 URL: sagemath#37262 Reported by: Aman Moon Reviewer(s): Sebastian Oehms
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Apr 20, 2024
I have implemented the `automatic size labeler`, which now assigns labels to pull requests based on the number of lines changed **Minimal** Typically involves very small changes, bug fixes, or updates that require only a few lines of code, often less than 50. sagemath#37208 sagemath#37146 sagemath#37043 **Small** Involves more substantial changes than minimal, potentially adding new features or making modifications to existing ones. The range is usually between 50 to 100 lines of code. sagemath#37152 sagemath#37132 **Moderate** Represents a significant portion of the codebase being modified, such as adding new features, refactoring, or making extensive changes to existing functionalities. This might involve between 100 to 300 lines of code. sagemath#36919 sagemath#37112 **Large** Involves substantial and complex changes across various parts of the codebase. This could include major architectural changes, the introduction of new modules, or a significant overhaul of existing features, often exceeding 300 lines of code. sagemath#37125 sagemath#36977 sagemath#36972 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. Fixes: sagemath#37254 URL: sagemath#37262 Reported by: Aman Moon Reviewer(s): Sebastian Oehms
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Apr 25, 2024
I have implemented the `automatic size labeler`, which now assigns labels to pull requests based on the number of lines changed **Minimal** Typically involves very small changes, bug fixes, or updates that require only a few lines of code, often less than 50. sagemath#37208 sagemath#37146 sagemath#37043 **Small** Involves more substantial changes than minimal, potentially adding new features or making modifications to existing ones. The range is usually between 50 to 100 lines of code. sagemath#37152 sagemath#37132 **Moderate** Represents a significant portion of the codebase being modified, such as adding new features, refactoring, or making extensive changes to existing functionalities. This might involve between 100 to 300 lines of code. sagemath#36919 sagemath#37112 **Large** Involves substantial and complex changes across various parts of the codebase. This could include major architectural changes, the introduction of new modules, or a significant overhaul of existing features, often exceeding 300 lines of code. sagemath#37125 sagemath#36977 sagemath#36972 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. Fixes: sagemath#37254 URL: sagemath#37262 Reported by: Aman Moon Reviewer(s): Sebastian Oehms
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New methods for
QuaternionFractionalIdeal_rational:QuaternionFractionalIdeal_rational.is_integral()QuaternionFractionalIdeal_rational.is_primitive()QuaternionFractionalIdeal_rational.primitive_decomposition()These methods verify if an ideal in quaternion rational algebra is integral, primitive (cyclic) and decompose it as a primitive ideal. Part of the code came from https://learningtosqi.github.io.
Also added the Quaternion Algebras book by J. Voight in the bibliography.
Done with @gioella
#sd123