Skip to content

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

Merged
vbraun merged 2 commits intosagemath:developfrom
GiacomoPope:fix_quaternion_37488
Mar 31, 2024
Merged

Fix failing docstring in random testing for quaternion_algebra.py#37489
vbraun merged 2 commits intosagemath:developfrom
GiacomoPope:fix_quaternion_37488

Conversation

@GiacomoPope
Copy link
Copy Markdown
Contributor

@GiacomoPope GiacomoPope commented Feb 27, 2024 •
edited
Loading

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 GiacomoPope mentioned this pull request Feb 27, 2024
Merged
@GiacomoPope GiacomoPope requested a review from yyyyx4 February 27, 2024 11:57
@GiacomoPope
Copy link
Copy Markdown
Contributor Author

GiacomoPope commented Feb 27, 2024

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.

yyyyx4
yyyyx4 approved these changes Feb 27, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

GiacomoPope commented Feb 27, 2024

Thanks Lorenz :)

@yyyyx4
Copy link
Copy Markdown
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 GiacomoPope mentioned this pull request Mar 1, 2024
Merged
5 tasks
@GiacomoPope
Copy link
Copy Markdown
Contributor Author

GiacomoPope commented Mar 13, 2024

@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?

@GiacomoPope GiacomoPope mentioned this pull request Mar 13, 2024
Merged
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 29, 2024

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
@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

Reviewers

1 more reviewer

@yyyyx4 yyyyx4 yyyyx4 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: algebra p: CI fix merged before running CI tests t: bug t: tests

Projects

None yet

Milestone

sage-10.4

Development

Successfully merging this pull request may close these issues.

Random test failure in quaternion_algebra.py

5 participants

@GiacomoPope @yyyyx4 @vbraun @mkoeppe @grhkm21