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

Don't call sylvester_matrix with zero polynomials #35449

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

tornaria
Copy link
Contributor

@tornaria tornaria commented Apr 6, 2023

📚 Description

From #35441 (comment):

$ sage -t --random-seed=3462219213258779680603652694803022552 /usr/lib/python3.11/site-packages/sage/rings/polynomial/multi_polynomial.pyx
too many failed tests, not using stored timings
Running doctests with ID 2023-04-06-17-14-49-7f2db03c.
Running with SAGE_LOCAL='/usr' and SAGE_VENV='/usr'
Using --optional=pip,sage
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,cvxopt,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_cubic_hecke,database_jones_numfield,database_knotinfo,dvipng,gfan,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,msolve,nauty,palp,pandoc,pdf2svg,pdftocairo,phitigra,plantri,polytopes_db,polytopes_db_4d,pynormaliz,python_igraph,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.misc.cython,sage.plot,sage.rings.number_field,sage.rings.padics,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib
Doctesting 1 file.
sage -t --random-seed=3462219213258779680603652694803022552 /usr/lib/python3.11/site-packages/sage/rings/polynomial/multi_polynomial.pyx
**********************************************************************
File "/usr/lib/python3.11/site-packages/sage/rings/polynomial/multi_polynomial.pyx", line 1390, in sage.rings.polynomial.multi_polynomial.MPolynomial.sylvester_matrix
Failed example:
    f.sylvester_matrix(g, x).determinant() == f.resultant(g, x)
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.11/site-packages/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.11/site-packages/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.polynomial.multi_polynomial.MPolynomial.sylvester_matrix[9]>", line 1, in <module>
        f.sylvester_matrix(g, x).determinant() == f.resultant(g, x)
        ^^^^^^^^^^^^^^^^^^^^^^^^
      File "sage/rings/polynomial/multi_polynomial.pyx", line 1474, in sage.rings.polynomial.multi_polynomial.MPolynomial.sylvester_matrix (build/cythonized/sage/rings/polynomial/multi_polynomial.c:17071)
        raise ValueError("The Sylvester matrix is not defined for zero polynomials")
    ValueError: The Sylvester matrix is not defined for zero polynomials
**********************************************************************
1 item had failures:
   1 of  36 in sage.rings.polynomial.multi_polynomial.MPolynomial.sylvester_matrix
    [577 tests, 1 failure, 3.72 s]
----------------------------------------------------------------------
sage -t --random-seed=3462219213258779680603652694803022552 /usr/lib/python3.11/site-packages/sage/rings/polynomial/multi_polynomial.pyx  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 8.8 seconds
    cpu time: 3.5 seconds
    cumulative wall time: 3.7 seconds
Features detected for doctesting: 

This is caused by a (random) polynomial that happens to be zero, which the sylvester_matrix() doesn't allow.

This PR fixes this so when R.random_element(4) returns 0, it uses x^2 * y^2 instead.

📝 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 are existing tests covering the changes.

@tornaria
Copy link
Contributor Author

tornaria commented Apr 6, 2023

Lint failures are exclusively due to #35418.

@tornaria tornaria force-pushed the fix-doctest-sylvester_matrix branch from 8f97610 to fc3735d Compare April 6, 2023 23:39
@tornaria
Copy link
Contributor Author

tornaria commented Apr 6, 2023

Rebased to 10.0.beta8

@github-actions
Copy link

github-actions bot commented Apr 7, 2023

Documentation preview for this PR is ready! 🎉
Built with commit: fc3735d

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

LGTM.

@vbraun vbraun merged commit 358d64d into sagemath:develop Apr 13, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 14, 2023
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.

4 participants