compute isomorphisms between quaternion orders

[yyyyx4]

Fixes #34861.

[codecov-commenter]

Codecov Report

Base: 88.57% // Head: 88.59% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (e265c3f) compared to base (52a81cb).
Patch coverage: 85.00% of modified lines in pull request are covered.

❗ Current head e265c3f differs from pull request most recent head 113d1bf. Consider uploading reports for the commit 113d1bf to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #34976      +/-   ##
===========================================
+ Coverage    88.57%   88.59%   +0.02%     
===========================================
  Files         2140     2136       -4     
  Lines       397273   396159    -1114     
===========================================
- Hits        351891   350991     -900     
+ Misses       45382    45168     -214     

Impacted Files	Coverage Δ
src/sage/algebras/quatalg/quaternion_algebra.py	92.73% <85.00%> (+0.04%)	⬆️
src/sage/crypto/util.py	91.37% <0.00%> (-5.18%)	⬇️
...c/sage/schemes/elliptic_curves/ell_finite_field.py	87.57% <0.00%> (-1.27%)	⬇️
src/sage/cpython/_py2_random.py	75.20% <0.00%> (-1.24%)	⬇️
src/sage/categories/posets.py	98.76% <0.00%> (-1.24%)	⬇️
src/sage/combinat/matrices/hadamard_matrix.py	84.43% <0.00%> (-1.01%)	⬇️
src/sage/modular/modform/numerical.py	94.19% <0.00%> (-0.65%)	⬇️
src/sage/combinat/schubert_polynomial.py	91.66% <0.00%> (-0.65%)	⬇️
...sage/geometry/hyperbolic_space/hyperbolic_model.py	88.95% <0.00%> (-0.62%)	⬇️
src/sage/categories/weyl_groups.py	96.47% <0.00%> (-0.59%)	⬇️
... and 97 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[GiacomoPope]

Looking through this code, it seems to be a good addition, even if only working for maximal orders currently. I see other reviewers on the list but no comments. I personally think this should be included.

[S17A05]

I wanted to look through the code later today, unfortunately I couldn't find time for it earlier this week.

[S17A05]

Sorry for the delay. It's just two small things, otherwise it looks good to me.

[S17A05]

Isn't it a bit more efficient to check whether the associated quaternion algebra is definite by checking the invariants as done in #37100? If so, I could put a dependency on that PR and update it accordingly after this PR is merged.

[yyyyx4]

Sounds good to me. In hindsight we should've probably just made one big pull request together, but alas that's not how things evolved...

[S17A05]

Yeah, we probably should have started directy from your code in #37100, but I'll fix that

[S17A05]

Just briefly resetting to "needs review" for the above two things.

[S17A05]

Also, while I don't think there should be any conflicts with the current version of develop, can you bring this branch up to date to make sure?

[S17A05]

Looks good to me now :)

[yyyyx4]

Thanks!

[github-actions]

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

[jtcc2]

I'm curious as to why in the random tests you ensure gcd(b.reduced_norm(), Quat.discriminant()) == 1 ?

For instance the following edge case doesn't work, which I would interpret as a bug.

p = 419
B.<i,j,k> = QuaternionAlgebra(p)
O = B.quaternion_order([1/2 + 3/2*j + k, 1/18*i + 25/9*j + 5/6*k, 3*j + 2*k, 3*k])
Oconj = j.inverse() * O * j
Oconj = B.quaternion_order(Oconj.basis())
O.isomorphism_to(Oconj)

It gives a ValueError: quaternion orders not isomorphic, when they are isomorphic.

And a small suggestion is if the two input orders are not isomorphic, you could return the error faster if you did a theta series check on the ideal $I = N O_1 O_2$ - it is not principal if the norm form represents anything which is not a square. I imagine running I.minimal_element() immediately would be slower. (e.g. a bit like this is_equivalent test)

I.theta_series_vector(10) == vector([1, 2, 0, 0, 2, 0, 0, 0, 0, 2])

[S17A05]

I'm curious as to why in the random tests you ensure gcd(b.reduced_norm(), Quat.discriminant()) == 1 ?

For instance the following edge case doesn't work, which I would interpret as a bug.

p = 419
B.<i,j,k> = QuaternionAlgebra(p)
O = B.quaternion_order([1/2 + 3/2*j + k, 1/18*i + 25/9*j + 5/6*k, 3*j + 2*k, 3*k])
Oconj = j.inverse() * O * j
Oconj = B.quaternion_order(Oconj.basis())
O.isomorphism_to(Oconj)

It gives a ValueError: quaternion orders not isomorphic, when they are isomorphic.

And a small suggestion is if the two input orders are not isomorphic, you could return the error faster if you did a theta series check on the ideal I=NO1O2 - it is not principal if the norm form represents anything which is not a square. I imagine running I.minimal_element() immediately would be slower. (e.g. a bit like this is_equivalent test)

I.theta_series_vector(10) == vector([1, 2, 0, 0, 2, 0, 0, 0, 0, 2])

You're right on that first point, I missed that in my review. The issue seems to be that the connecting ideal is only locally principal by the results in Voight, and one cannot get global principality from that if the norm of the conjugating element is divisible by a ramified prime of the algebra; I believe this comes down to the proofs of the statements (b) and (c) in Exercise 4 in Chapter 17 of Voight's book. So the orders are still isomorphic, but trying to find a global generator of the connecting ideal fails and the method gives the wrong result.

[S17A05]

I'm curious as to why in the random tests you ensure gcd(b.reduced_norm(), Quat.discriminant()) == 1 ?
For instance the following edge case doesn't work, which I would interpret as a bug.

p = 419
B.<i,j,k> = QuaternionAlgebra(p)
O = B.quaternion_order([1/2 + 3/2*j + k, 1/18*i + 25/9*j + 5/6*k, 3*j + 2*k, 3*k])
Oconj = j.inverse() * O * j
Oconj = B.quaternion_order(Oconj.basis())
O.isomorphism_to(Oconj)

It gives a ValueError: quaternion orders not isomorphic, when they are isomorphic.
And a small suggestion is if the two input orders are not isomorphic, you could return the error faster if you did a theta series check on the ideal I=NO1O2 - it is not principal if the norm form represents anything which is not a square. I imagine running I.minimal_element() immediately would be slower. (e.g. a bit like this is_equivalent test)

I.theta_series_vector(10) == vector([1, 2, 0, 0, 2, 0, 0, 0, 0, 2])

You're right on that first point, I missed that in my review. The issue seems to be that the connecting ideal is only locally principal by the results in Voight, and one cannot get global principality from that if the norm of the conjugating element is divisible by a ramified prime of the algebra; I believe this comes down to the proofs of the statements (b) and (c) in Exercise 4 in Chapter 17 of Voight's book. So the orders are still isomorphic, but trying to find a global generator of the connecting ideal fails and the method gives the wrong result.

@yyyyx4 Unless there is an easy fix that I am missing, I figure it's best to make the error warning slightly more ambiguous for now, e.g. by letting the user know that the method is not able to detect whether the given orders are isomorphic, and to add this limitation to the docstring (maybe with the example that @jtcc2 gave above).