Quaternion Algebra Fractional Ideal improvements - equivalence and reduced bases #37100

jtcc2 · 2024-01-19T08:30:23Z

Includes the following changes

Added .is_definite() to quaternion algebras, which performs a check on the invariants.
Added .reduced_basis() to quaternion fractional ideals which uses LLL to find the basis of shortest elements
Modified quaternion fractional ideal .is_equivalent to include a side parameter which can be set to 'left' or 'right' to do left-ideal or right-ideal equivalence, and a certificate parameter which if true, returns the element a such that I = J*a. Throws a NotImplementedError for indefinite quaternion algebras which fixes Ideal equivalence in indefinite quaternion algebras #37080 . Backwards compatible.
Also added .is_left_equivalent and .is_right_equivalent.
Added .is_principal to quaternion fractional ideals of rank 4 in definite quaternion algebras. Includes certificate parameter for finding generator. Finds the shortest norm element in the lattice and checks its norm.
Fixes isomorphism_to of quaternion_algebra.py does not find an isomorphism of isomorphic maximal orders #37337, bug in quaternion order .isomorphism_to where it cannot find the isomorphism between $\mathcal{O}$ and $\alpha^{-1} \mathcal{O} \alpha$ if $nrd(\alpha)$ is a ramified prime times a square. Fixed this by repeating the previous isomorphism_to method multiple times on inputs $\mathcal{O}$ and $\alpha^{-1} \mathcal{O} \alpha$ with $\alpha = 1, i, j, k$. Throws a NotImplementedError in cases not covered.

@S17A05 @Jonathke

#sd123

S17A05 · 2024-01-30T16:51:11Z

@yyyyx4 Can you have a look at this since you worked on a similar issue in #34976?

yyyyx4 · 2024-02-02T14:25:48Z

src/sage/algebras/quatalg/quaternion_algebra.py

+ if not self.quaternion_algebra().is_definite():
+ if not self.quadratic_form().is_positive_definite():
+ raise TypeError("The quaternion algebra must be definite")


Aren't these two tests equivalent? Were they duplicated by accident?

Yes, these two should be equivalent - I believe the latter test was originally put in place for the method to also work for "lattices" of non-maximal rank, until it came up that .gram_matrix() needs a full rank lattice anyway. I'll remove the second test.

yyyyx4 · 2024-02-02T14:30:16Z

src/sage/algebras/quatalg/quaternion_algebra.py

 """
- # shorthand: let I be self
- if not isinstance(self, QuaternionFractionalIdeal_rational):
+ if not isinstance(J, QuaternionFractionalIdeal_rational):


I'd say in this case we should probably throw a TypeError. Returning False is arguably correct, but I can't see any situation in which users will rely on this behaviour wilfully, hence failing visibly is better.

yyyyx4 · 2024-02-02T14:31:48Z

src/sage/algebras/quatalg/quaternion_algebra.py

- def is_equivalent(self, J, B=10) -> bool:
- """
- Return ``True`` if ``self`` and ``J`` are equivalent as right ideals.
+ def is_equivalent(self, J, B=10, certificate=False, side=None):


Personally I would simply deprecate this method in favor of the two new explicitly named one-sided methods: Having it default to .is_right_equivalent() is arbitrary and ambiguous.

So should I add a deprecation warning for now to set it up for future removal, or should I remove it immediately?

According to The Rules, we're only allowed to remove things once they've been deprecated for one year, so it has to be the former: Add a deprecation warning. (If you agree, that is: I don't have a strong opinion about this either way.)

yyyyx4 · 2024-02-02T14:32:18Z

src/sage/algebras/quatalg/quaternion_algebra.py

+ raise NotImplementedError('equivalence test of ideals not implemented'
+ 'for indefinite quaternion algebras')


Suggested change

raise NotImplementedError('equivalence test of ideals not implemented'

'for indefinite quaternion algebras')

raise NotImplementedError('equivalence test of ideals not implemented'

' for indefinite quaternion algebras')

yyyyx4 · 2024-02-02T14:38:10Z

src/sage/algebras/quatalg/quaternion_algebra.py

+ # Use Pari's qfminim with flag = 1 to find alpha with N(alpha) = N(I)
+ _,v = self.quadratic_form().__pari__().qfminim(None, None, 1)
+ return True, sum(ZZ(c)*g for c,g in zip(v, self.basis()))


Same here re: the .minimal_element() method from #34976.

yyyyx4 · 2024-02-02T14:39:13Z

src/sage/algebras/quatalg/quaternion_algebra.py

+ 'implemented in indefinite'
+ 'quaternion algebras')
+
+ if len(self.basis()) < 4:


Can this fail?

On faulty input, I believe, since calling B.ideal([i,j]) for a quaternion algebra B gives a "fractional ideal", but it really is just a two-dimensional sublattice with basis (i,j).

Interesting, I was expecting the constructor for fractional ideals to check the rank. That should probably be fixed eventually, but it doesn't necessarily have to be done here and now.

yyyyx4 · 2024-02-02T14:39:31Z

src/sage/algebras/quatalg/quaternion_algebra.py

+ raise NotImplementedError('principality test not'
+ 'implemented in indefinite'
+ 'quaternion algebras')


Suggested change

raise NotImplementedError('principality test not'

'implemented in indefinite'

'quaternion algebras')

raise NotImplementedError('principality test not'

' implemented in indefinite'

' quaternion algebras')

yyyyx4 · 2024-02-02T14:39:48Z

src/sage/algebras/quatalg/quaternion_algebra.py

+ # Use Pari's qfminim with flag = 1 to find alpha as above
+ _,v = IJbar.quadratic_form().__pari__().qfminim(None, None, 1)
+ alpha = sum(ZZ(c)*g for c,g in zip(v, IJbar.basis()))


This one could be alpha = IJbar.minimal_element() once #34976 is merged, but pulling that in as a dependency seems a bit overkill just to deduplicate these two lines. Maybe add a TODO so that eventually someone can replace it later?

Since it might be a good idea to test definiteness with this PR's .is_definite()-method as well, it might actually be a good idea to put a dependency on this PR to yours (see also this review comment).

yyyyx4 · 2024-02-02T14:41:50Z

src/sage/algebras/quatalg/quaternion_algebra.py

+ # As explained by Pizer, we now only need to rescale alpha by 1/N(J)
+ return True, alpha * (1/J.norm())
+
+ def is_principal(self, certificate=False):


This method duplicates quite some code from the equivalence tests. Perhaps the equivalence tests could/should just end up calling .is_principal() on IJbar instead of repeating all the steps on that special case?

S17A05 · 2024-02-02T18:14:14Z

@yyyyx4 Once #34976 is merged I will modify the definiteness check in your code and will integrate your .minimal_element()-method into our code, and then I'll re-request a review; for now I'll just leave this PR labelled as "needs work".

S17A05 · 2024-02-03T01:18:40Z

Since beta 7 of Sage 10.3 was just released, I have instead decided to merge #34976 into this branch. In the progress of figuring this out, however, I caused some chaos in the PR; in particular, GitHub now thinks that a lot of files in .devcontainer have been modified, even though they should be the same files - if someone knows how to fix this please let me know, so that I can repair it.
Otherwise this should be ready for a second review, so I'll update the labels.

fchapoton · 2024-02-03T09:58:05Z

it was not a good idea to merge with the latest develop. One better way if you really want that is to use github's update branch button that does the merge in the correct direction.

To fix the merge, you could try to rebase your branch on top of develop using git rebase -i develop, but this may trigger conflicts.

S17A05 · 2024-02-03T12:20:57Z

it was not a good idea to merge with the latest develop. One better way if you really want that is to use github's update branch button that does the merge in the correct direction.

To fix the merge, you could try to rebase your branch on top of develop using git rebase -i develop, but this may trigger conflicts.

That's how I did it originally, but then I wrongly assumed that I had to revert it to merge with the other PR's branch, and after that I couldn't do it this way again.

Thanks for the suggestion, I'll try to do that.

S17A05 · 2024-02-03T12:29:07Z

I rebased the branch on upstream/develop, but it still lists the .devcontainer-files under the made changes.

…(), and is_equivalent certificate

- Removed greek letter alpha in docstrings in hopes of this fixing infinite build loops - Updated `.is_definite()` to fit PR sagemath#37173 (up to the reference to Voight's book) - Other small modifications of docstrings, comments and error warnings - Slightly cleaned up code with respect to intermediately defined variables

- Added deprecation warning for `.is_equivalent` - Removed unnecessary call of `.is_positive_definite()` in `.reduced_basis()` - Reduced `.is_right_equivalent` to `.is_principal` with suitable rescaling - Fixed some error warnings - Modified docstrings, moved examples to non-deprecated methods and randomized test of `.is_principal`

- Modified definiteness check in `.minimal_element()` and `.isomorphism_to`; also added check of the base field in the latter method - Reduced search for a generator in `.is_principal()` to a call to `.minimal_element()`

yyyyx4 · 2024-02-05T15:12:51Z

The merge still seems broken. I've attempted to fix it on a new branch by cherry-picking all non-merge commits from here on top of the current develop branch.

You can get my version by adding my fork as a remote and overwriting your version of this branch with mine:

git remote add yyyyx4 https://github.com/yyyyx4/sage.git
git fetch yyyyx4 quatfracidl:this-branch-name -f

After that, you should be able to git push -f the new version of the branch to this pull request as usual.

S17A05 · 2024-02-05T19:06:44Z

The merge still seems broken. I've attempted to fix it on a new branch by cherry-picking all non-merge commits from here on top of the current develop branch.

You can get my version by adding my fork as a remote and overwriting your version of this branch with mine:
git remote add yyyyx4 https://github.com/yyyyx4/sage.git
git fetch yyyyx4 quatfracidl:this-branch-name -f
After that, you should be able to git push -f the new version of the branch to this pull request as usual.

Perfect, that worked - thanks for the help!

This is not a fix for the issue - it just adapts the error warning to be more ambiguous, as suggested in sagemath#34976 (comment)

github-actions · 2024-03-08T23:54:25Z

Documentation preview for this PR (built with commit 8d75358; changes) is ready! 🎉

yyyyx4

Looks good to me now, thanks!

yyyyx4 added p: major / 3 t: enhancement s: needs review c: algebra labels Jan 19, 2024

S17A05 mentioned this pull request Jan 27, 2024

Implemented .ramified_places and modified further methods to extend quaternion algebra functionality to number fields #37173

Open

S17A05 requested a review from yyyyx4 January 30, 2024 16:49

yyyyx4 reviewed Feb 2, 2024

View reviewed changes

yyyyx4 added s: needs work and removed s: needs review labels Feb 2, 2024

S17A05 mentioned this pull request Feb 2, 2024

compute isomorphisms between quaternion orders #34976

Merged

S17A05 force-pushed the develop branch 2 times, most recently from b65ec82 to 554e920 Compare February 3, 2024 00:40

S17A05 added s: needs review and removed s: needs work labels Feb 3, 2024

S17A05 requested a review from yyyyx4 February 3, 2024 01:20

S17A05 force-pushed the develop branch from be80895 to bd767ca Compare February 3, 2024 12:24

jtcc2 added 8 commits February 5, 2024 16:01

Added QuaternionAlgebra.is_definite(), Quaternion Ideal reduced_basis…

f0f9d5a

…(), and is_equivalent certificate

Improved definiteness check for reduced_basis

180618e

reduced_basis must have full rank ideal

fc76364

Removed whitespace

f48b3db

Added .is_principal

e234f7a

Fixed codestyle

9372816

Fixed codestyle

1a21d10

fix

e49fc7e

S17A05 and others added 8 commits February 5, 2024 16:01

Fix lint issues

d55b29a

(Attempt to) Fix deprecation failure

039d80e

Replace is_equivalent in BrandtModule

099b8c9

Merge branch 'public/isomorphisms_between_quaternion_orders'

2524b96

Fully integrated sagemath#34976

094a40f

- Modified definiteness check in `.minimal_element()` and `.isomorphism_to`; also added check of the base field in the latter method - Reduced search for a generator in `.is_principal()` to a call to `.minimal_element()`

Added check for fractional ideal basis length, removed "local" checks

abb108b

S17A05 force-pushed the develop branch from bd767ca to abb108b Compare February 5, 2024 19:05

S17A05 and others added 2 commits February 14, 2024 11:36

Merge branch 'develop' into develop

93f55e2

Address issue sagemath#37337

1ceae66

This is not a fix for the issue - it just adapts the error warning to be more ambiguous, as suggested in sagemath#34976 (comment)

S17A05 force-pushed the develop branch from 54b2c5e to 1ceae66 Compare February 14, 2024 16:13

jtcc2 and others added 8 commits February 15, 2024 19:17

Fixed isomorphism_to bug

5617256

Fixed lint

cf070c9

Added tests

57d933a

Added tests

b7778fd

Small correction

e5407af

Merge branch 'develop' into develop

a16a934

Merge branch 'develop' into develop

ec581a4

Merge branch 'develop' into develop

8d75358

yyyyx4 added s: positive review and removed s: needs review labels Mar 18, 2024

yyyyx4 approved these changes Mar 18, 2024

View reviewed changes

vbraun merged commit 9d73e3c into sagemath:develop Mar 25, 2024
15 of 16 checks passed

mkoeppe added this to the sage-10.4 milestone Mar 25, 2024

github-actions bot removed the s: positive review label Mar 26, 2024

S17A05 mentioned this pull request Jul 26, 2024

Quaternion ideal behaviour changed in 10.4 #38422

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quaternion Algebra Fractional Ideal improvements - equivalence and reduced bases #37100

Quaternion Algebra Fractional Ideal improvements - equivalence and reduced bases #37100

jtcc2 commented Jan 19, 2024 •

edited

Loading

S17A05 commented Jan 30, 2024 •

edited

Loading

yyyyx4 Feb 2, 2024

S17A05 Feb 2, 2024

yyyyx4 Feb 2, 2024

yyyyx4 Feb 2, 2024

S17A05 Feb 2, 2024

yyyyx4 Feb 2, 2024

yyyyx4 Feb 2, 2024

yyyyx4 Feb 2, 2024

yyyyx4 Feb 2, 2024

S17A05 Feb 2, 2024

yyyyx4 Feb 2, 2024

yyyyx4 Feb 2, 2024

yyyyx4 Feb 2, 2024

S17A05 Feb 2, 2024 •

edited

Loading

yyyyx4 Feb 2, 2024

S17A05 commented Feb 2, 2024

S17A05 commented Feb 3, 2024 •

edited

Loading

fchapoton commented Feb 3, 2024

S17A05 commented Feb 3, 2024

S17A05 commented Feb 3, 2024

yyyyx4 commented Feb 5, 2024

S17A05 commented Feb 5, 2024

github-actions bot commented Mar 8, 2024

yyyyx4 left a comment

		raise NotImplementedError('equivalence test of ideals not implemented'
		'for indefinite quaternion algebras')

Quaternion Algebra Fractional Ideal improvements - equivalence and reduced bases #37100

Quaternion Algebra Fractional Ideal improvements - equivalence and reduced bases #37100

Conversation

jtcc2 commented Jan 19, 2024 • edited Loading

S17A05 commented Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S17A05 Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S17A05 commented Feb 2, 2024

S17A05 commented Feb 3, 2024 • edited Loading

fchapoton commented Feb 3, 2024

S17A05 commented Feb 3, 2024

S17A05 commented Feb 3, 2024

yyyyx4 commented Feb 5, 2024

S17A05 commented Feb 5, 2024

github-actions bot commented Mar 8, 2024

yyyyx4 left a comment

Choose a reason for hiding this comment

jtcc2 commented Jan 19, 2024 •

edited

Loading

S17A05 commented Jan 30, 2024 •

edited

Loading

S17A05 Feb 2, 2024 •

edited

Loading

S17A05 commented Feb 3, 2024 •

edited

Loading