Skip to content

Handle empty matrices over univariate polynomials #39852

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

Merged
merged 6 commits into from
Apr 29, 2025

Conversation

Biffo89
Copy link
Contributor

@Biffo89 Biffo89 commented Apr 2, 2025

Fixes #39587

Adds handling of empty matrices over univariate polynomials, specifically in the following methods:

  • degree() (returns -1)
  • constant_matrix()
  • inverse_series_trunc()
  • solve_left_series_trunc()
  • row_degrees() ([] if no rows, [-1, ..., -1] if no columns)
  • column_degrees() (vice versa)
  • is_hermite() (defaults to _is_empty_popov())
  • weak_popov_form()
  • right_quo_rem()
  • _right_quo_rem_reduced()
  • minimal_relation_basis()

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

@Biffo89 Biffo89 changed the title 39587 Handle empty matrices over univariate polynomials Apr 2, 2025
Copy link

github-actions bot commented Apr 2, 2025

Documentation preview for this PR (built with commit a6bbe0f; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@vneiger vneiger self-assigned this Apr 2, 2025
@Biffo89 Biffo89 force-pushed the 39587 branch 2 times, most recently from 55469f5 to e776011 Compare April 6, 2025 15:18
@@ -261,7 +256,7 @@ cdef class Matrix_polynomial_dense(Matrix_generic_dense):
[4 1 5 6]
"""
from sage.matrix.constructor import matrix
return matrix([[self[i,j].constant_coefficient()
return matrix(self.base_ring().base_ring(), [[self[i,j].constant_coefficient()
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. I assume that without this, this could return a matrix over QQ if self has zero rows or columns. Which could create problems if this now-rational matrix is used later in other operations.

@vneiger
Copy link
Contributor

vneiger commented Apr 6, 2025

Thanks a lot for the work, this fixes the "empty matrix issue" in almost all methods. When creating #39587 I was not sure whether empty matrices should be supported by all methods (e.g. row_degrees or leading_positions), but now I do not see why not, this all seems to fit nicely.

I write "almost all" because:

sage: ring.<x> = GF(97)[]
sage: M = matrix(ring, 2, 0)
sage: B = matrix(ring, 1, 0)
sage: M.reduce(B)
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
.....
IndexError: matrix index out of range

This should be fixed before merging.

@vneiger
Copy link
Contributor

vneiger commented Apr 6, 2025

Could someone with the relevant rights allow the pending checks to run, maybe @fchapoton ? Thanks a lot.

@vneiger
Copy link
Contributor

vneiger commented Apr 6, 2025

Another minor issue, to be investigated:

sage: ring.<x> = GF(97)[]
sage: mat = matrix(ring, 4, 0)
sage: mat.is_popov(include_zero_vectors=False)
True

this matrix has 4 rows and 0 columns, it should not be considered in Popov form (Popov form requires full row rank, if zero rows are not allowed). Strangely, there is no similar issue for is_hermite.

@vneiger
Copy link
Contributor

vneiger commented Apr 6, 2025

Another minor issue, to be investigated:

sage: ring.<x> = GF(97)[]
sage: mat = matrix(ring, 4, 0)
sage: mat.is_popov(include_zero_vectors=False)
True

this matrix has 4 rows and 0 columns, it should not be considered in Popov form (Popov form requires full row rank, if zero rows are not allowed). Strangely, there is no similar issue for is_hermite.

In fact, this has nothing to do with the modifications in this PR. But may as well be fixed at the same time. @Biffo89 could you please replace the line

            return self._is_empty_popov(row_wise)

in both def is_popov and def is_weak_popov, by the line:

            return self._is_empty_popov(row_wise, include_zero_vectors)

? Thank you.

@vneiger
Copy link
Contributor

vneiger commented Apr 22, 2025

Thanks for the fixes. In fact this also fixes the other above mentioned issue (which was a consequence of the wrong result of is_popov called in method reduce with include_zero_vectors=False):

sage: ring.<x> = GF(97)[]
sage: M = matrix(ring, 2, 0)
sage: B = matrix(ring, 1, 0)
sage: M.reduce(B)
[]
sage: M.reduce(B).dimensions()
(2, 0)

Now everything looks good to me.

@vneiger vneiger removed their assignment Apr 22, 2025
@vbraun vbraun merged commit ac81b0b into sagemath:develop Apr 29, 2025
16 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Matrices over the univariate polynomials: unify the handling of "empty" matrices
4 participants