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

failing doctest on Apple M1: corrected the test case by sorting the result #36852

Merged
merged 4 commits into from
Dec 14, 2023

Conversation

amanmoon
Copy link
Contributor

I have added rts = sorted(rts, key=lambda x: (x.real(), x.imag()))
after rts = f.roots(ring=fld_out, multiplicities=False) in

for (fld_in, fld_out) in flds:
        x = polygen(fld_in)
        f = x^3 - fld_in(2)
        x2 = polygen(fld_out)
        f2 = x2^3 - fld_out(2)
        for algo in (None, 'pari', 'numpy'):
            rts = f.roots(ring=fld_out, multiplicities=False)
            if fld_in == fld_out and algo is None:
                print("{} {}".format(fld_in, rts))
            for rt in rts:
                assert(abs(f2(rt)) <= 1e-10)
                assert(rt.parent() == fld_out)

I have sorted the rts; first based on the real part of the complex number and then based on the complex part.
I have also updated the test result by sorting them aswell.
This ensures that the output matches the test results, resulting in a passing test.

Fixes #36850

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion. there where also many

@amanmoon amanmoon changed the title corrected the test case by sorting the result failing doctest on Apple M1: corrected the test case by sorting the result Dec 10, 2023
@dimpase
Copy link
Member

dimpase commented Dec 11, 2023

Thanks, and welcome to the weird world of floating point computations. 😃

As you can see in the failing CI run you get the doctest error there.

I think the reason is that the real parts of the roots you are sorting are not exactly equal, and so your lexicographic ordering does not work as you wanted. Yes, in principle you could do rounding in your lambda-function:
key=lambda x: (x.real(), x.imag()).

But in this particular case it would suffice to sort on the imaginary part only (as they are quite different for the 3 roots),
so this would only make the code simpler, and correct, too.

File "sage/rings/polynomial/polynomial_element.pyx", line 8101, in sage.rings.polynomial.polynomial_element.Polynomial.roots
Failed example:
    for (fld_in, fld_out) in flds:
        x = polygen(fld_in)
        f = x^3 - fld_in(2)
        x2 = polygen(fld_out)
        f2 = x2^3 - fld_out(2)
        for algo in (None, 'pari', 'numpy'):
            rts = f.roots(ring=fld_out, multiplicities=False)
            rts = sorted(rts, key=lambda x: (x.real(), x.imag()))
            if fld_in == fld_out and algo is None:
                print("{} {}".format(fld_in, rts))
            for rt in rts:
                assert(abs(f2(rt)) <= 1e-10)
                assert(rt.parent() == fld_out)
Expected:
    Real Field with 53 bits of precision [1.25992104989487]
    Real Double Field [1.25992104989...]
    Real Field with 100 bits of precision [1.2599210498948731647672106073]
    Complex Field with 53 bits of precision [-0.62996052494743... - 1.09112363597172*I, -0.62996052494743... + 1.09112363597172*I, 1.25992104989487]
    Complex Double Field [-0.629960524947... - 1.0911236359717...*I, -0.629960524947... + 1.0911236359717...*I, 1.25992104989...]
    Complex Field with 100 bits of precision [-0.62996052494743658238360530364 - 1.0911236359717214035600726142*I, -0.62996052494743658238360530364 + 1.0911236359717214035600726142*I, 1.2599210498948731647672106073]
Got:
    Real Field with 53 bits of precision [1.25992104989487]
    Real Double Field [1.2599210498948734]
    Real Field with 100 bits of precision [1.2599210498948731647672106073]
    Complex Field with 53 bits of precision [-0.629960524947437 - 1.09112363597172*I, -0.629960524947437 + 1.09112363597172*I, 1.25992104989487]
    Complex Double Field [-0.6299605249474365 + 1.0911236359717214*I, -0.6299605249474364 - 1.0911236359717214*I, 1.259921049894873]
    Complex Field with 100 bits of precision [-0.62996052494743658238360530364 - 1.0911236359717214035600726142*I, -0.62996052494743658238360530364 + 1.0911236359717214035600726142*I, 1.2599210498948731647672106073]

@amanmoon amanmoon force-pushed the bug/failing_doctest_swapped_roots branch from c8750a8 to a447432 Compare December 11, 2023 17:39
@amanmoon
Copy link
Contributor Author

I rounded off the values in the Complex Double Field to correct the test cases.

@dimpase
Copy link
Member

dimpase commented Dec 12, 2023

ok, this works on Apple M1

Copy link

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

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

looks good now.

@vbraun vbraun merged commit 033d95a into sagemath:develop Dec 14, 2023
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 14, 2023
@amanmoon amanmoon deleted the bug/failing_doctest_swapped_roots branch December 15, 2023 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

failing doctest on Apple M1 due to swapped order of roots
4 participants