Corrections in .normalize_basis_at_p and .maximal_order() of quaternion_algebra.py
#37644
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
... and additional fix in `.maximal_order()`. Amend: Added a space in docstring
S17A05
force-pushed
the
normalize_maximal_fix
branch
from
4f81ab7
to
fb7b560
Compare
S17A05
force-pushed
the
normalize_maximal_fix
branch
from
69bca0d
to
57532ff
Compare
... plus small refactoring of code
S17A05
force-pushed
the
normalize_maximal_fix
branch
from
b21fa3b
to
08004b7
Compare
mkoeppe
reviewed
Apr 3, 2024
mkoeppe
reviewed
Apr 3, 2024
mkoeppe
reviewed
Apr 3, 2024
- Blank spaces in error warning - Matrix definition - List comprehension for discriminant computation, also updated in `.discriminant()` of an order
|
Documentation preview for this PR (built with commit 792a98c; changes) is ready! 🎉 |
4 tasks
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Apr 20, 2024
… to extend a given order
1. Added an optional argument `order_basis` to `.maximal_order()` to
allow for extension of orders: If `order_basis` defines an order of the
given quaternion algebra, then the method now returns a maximal order
that contains the order specified by the given basis.
~~2. Added comparison functions `__le__`, `__ge__`, `__lt__` and
`__gt__` for quaternion orders.~~
~~3. Small edits of the comparison functions `__eq__` and `__ne__` for
quaternion orders.~~
2. Added a general comparison function `__richcmp__` for quaternion
orders and removed the explicit implementation of `__eq__` and `__ne__`.
More details on 1.:
The algorithm used in `.maximal_order()` currently uses the standard
basis `(1,i,j,k)` as a starting basis, but it extends to any starting
basis whose first entry is equal to `1`. Using the helper method
`basis_for_quaternion_lattice`, we transform any quaternion order basis
input by the user to a basis of this form.
Depends on sagemath#37644: Naturally, the extended algorithm can only work
correctly if the base algorithm works correctly in the first place.
URL: sagemath#37675
Reported by: Sebastian A. Spindler
Reviewer(s): Sebastian A. Spindler, Travis Scrimshaw
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Apr 25, 2024
… to extend a given order
1. Added an optional argument `order_basis` to `.maximal_order()` to
allow for extension of orders: If `order_basis` defines an order of the
given quaternion algebra, then the method now returns a maximal order
that contains the order specified by the given basis.
~~2. Added comparison functions `__le__`, `__ge__`, `__lt__` and
`__gt__` for quaternion orders.~~
~~3. Small edits of the comparison functions `__eq__` and `__ne__` for
quaternion orders.~~
2. Added a general comparison function `__richcmp__` for quaternion
orders and removed the explicit implementation of `__eq__` and `__ne__`.
More details on 1.:
The algorithm used in `.maximal_order()` currently uses the standard
basis `(1,i,j,k)` as a starting basis, but it extends to any starting
basis whose first entry is equal to `1`. Using the helper method
`basis_for_quaternion_lattice`, we transform any quaternion order basis
input by the user to a basis of this form.
Depends on sagemath#37644: Naturally, the extended algorithm can only work
correctly if the base algorithm works correctly in the first place.
URL: sagemath#37675
Reported by: Sebastian A. Spindler
Reviewer(s): Sebastian A. Spindler, Travis Scrimshaw
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.
.normalize_basis_at_pthe wrong assignments off0, f1: The idea is to replacef0byf0 + f1andf1byf0simultaneously - the original code, however, first replacedf0byf0 + f1and then copied the value tof1, hence causing a decrease in dimensionnormalize_basis_at_pin the off-diagnonal case forp = 2(theelse-block).maximal_order()the intermediate basise_nmight not define an order anymore (as seen in the comments, this was known to the author(s) of the method) - therefore we need to manually compute the discriminant in the loop instead of relying on the.discriminant()-method of quaternion orders.maximal_order()to use the new method.is_maximal()Fixes #37217 (both parts) and #37417.
Disclaimer:
I am aware of #37239, but since progress on that PR seems to have halted, I took the liberty to fix the issues myself :)