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.

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

Fix failing docstring in random testing for quaternion_algebra.py #37489

Merged
merged 2 commits into from
Mar 31, 2024

Conversation

GiacomoPope
Copy link
Contributor

@GiacomoPope GiacomoPope commented Feb 27, 2024

The current code:

sage: set_random_seed(558346185206384723433409773377632267)
sage: q = randrange(1,1000)
....: p = randrange(1,1000)
....: Quat.<i,j,k> = QuaternionAlgebra(-q, -p)
....: O0 = Quat.maximal_order()
....: while True:
....:     b = Quat.random_element()
....:     if gcd(b.reduced_norm(), Quat.discriminant()) == 1:
....:         break
....: O1 = (b * O0).left_order()
....: iso = O0.isomorphism_to(O1); iso
Ring endomorphism of Order of Quaternion Algebra (-667, -662) with base ring Rational Field with basis (1, 1/2 + 1/2*i, j, 1/2 + 11/58*i + 1/2*j + 1/58*k)
  Defn: i |--> i
        j |--> j
        k |--> k
sage: O0
Order of Quaternion Algebra (-667, -662) with base ring Rational Field with basis (1, 1/2 + 1/2*i, j, 1/2 + 11/58*i + 1/2*j + 1/58*k)
sage: O1
Order of Quaternion Algebra (-667, -662) with base ring Rational Field with basis (1, 1/2 + 1/2*i, j, 1/2 + 11/58*i + 1/2*j + 1/58*k)

Causes a doctest to fail as a Ring morphism is expected but when O0 == O1 then instead a Ring endomorphism is computed instead.

We simply truncate the expected output for the random testing from Ring morphism to Ring ... fixing the failure.

sage -t --random-seed=558346185206384723433409773377632267 src/sage/algebras/quatalg/quaternion_algebra.py
    [562 tests, 17.35 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 18.7 seconds
    cpu time: 15.6 seconds
    cumulative wall time: 17.3 seconds

Fixes #37488

@GiacomoPope
Copy link
Contributor Author

I don't know whether to label this as minor, as it's a small bug, or blocker, as it currently results in CI failure.

Copy link
Member

@yyyyx4 yyyyx4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good enough to me. I guess blocker makes sense since it's a really trivial change (not much can go wrong by merging it), and the failure being present in a release could waste quite a few people's time.

@GiacomoPope
Copy link
Contributor Author

Thanks Lorenz :)

@yyyyx4
Copy link
Member

yyyyx4 commented Feb 28, 2024

Relabelling to minor per this sage-devel thread, but it would still be nice to get this into the release @vbraun.

@GiacomoPope
Copy link
Contributor Author

@vbraun following Lorenz's message -- this bug is causing repeated CI failures in my PR considering the tiny footprint of this PR it would be nice to get it included?

Copy link

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

@grhkm21 grhkm21 added p: CI Fix merged before running CI tests and removed p: minor / 4 labels Mar 29, 2024
@vbraun vbraun merged commit 1c3f991 into sagemath:develop Mar 31, 2024
15 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Mar 31, 2024
@GiacomoPope GiacomoPope deleted the fix_quaternion_37488 branch April 1, 2024 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: algebra p: CI Fix merged before running CI tests t: bug t: tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random test failure in quaternion_algebra.py
5 participants