vector.length() should be number of nonzero entries

[mwageringel]

sage: vector([-1, 0, 2, 0]).length()  # should be 2
4

According to the documentation of length from modules_with_basis.py:

Return the number of basis elements whose coefficients in "self" are nonzero.

This is solved by overwriting length in FreeModuleElement.

Depends on #34509

Component: linear algebra

Author: Markus Wageringel

Branch/Commit: u/mkoeppe/29218 @ 324478d

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/29218

[mwageringel]

New commits:

324478d

29218: overwrite .length() of FreeModuleElement

[mwageringel]

Author: Markus Wageringel

[mwageringel]

Branch: u/gh-mwageringel/29218

[mwageringel]

Commit: 324478d

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:2

Hmmm...my initial thought was this is a good change to match the behavior as documented. However, there is an issue that does arise. The iterator for a vector is different than for a more generic element of a ModuleWithBasis. In the former, it goes over all values (in part because the basis is finite and has a fixed order) whereas in the latter it just goes over the supported basis elements. So for the latter, x.length() == len(x.support()), but I could see for a vector v someone doing something like

for i in range(v.length()):
    v[i] += 1

So this could break code and it is somewhat unnatural given the name of the method. I would propose instead overriding the documentation to reflect the more natural behavior because of the implementation and how people are expecting to use this class. I further believe this will be a very special case to allow the break in inherited behavior.

[mwageringel]

comment:3

The method is also broken for matrices:

sage: matrix(QQ, 4).length()
...
TypeError: object of type 'sage.matrix.matrix_rational_dense.Matrix_rational_dense' has no len()

This would be useful for sparse matrices in particular.

What would you suggest to do with matrices then? Should it return the number of rows?

Given that the method does not currently work for matrices, it would not break anything if we make it return the length of the support, except that it is inconsistent with vectors.

[tscrim]

comment:4

There is much less of a case for matrices as it is only because of the internal representation as rows that length has a distinct meaning. There is nothing really special about the number of rows versus columns, and it makes equal sense to have it be the size of the support. So I would instead fix matrices to be the size of the support and keep vectors as something special (and inconsistent).

[mkoeppe]

comment:5

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

[mkoeppe]

comment:7

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:8

Setting a new milestone for this ticket based on a cursory review.

[mkoeppe]

comment:9

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

[mkoeppe]

comment:12

Replying to Markus Wageringel:

The method is also broken for matrices:

sage: matrix(QQ, 4).length()
...
TypeError: object of type 'sage.matrix.matrix_rational_dense.Matrix_rational_dense' has no len()

Still broken in 9.7.rc1. This must be some strange Cython-related effect:

sage: matrix(QQ, 4).length()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Input In [6], in <cell line: 1>()
----> 1 matrix(QQ, Integer(4)).length()

File ~/s/sage/sage-rebasing/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/categories/modules_with_basis.py:1531, in ModulesWithBasis.ElementMethods.length(self)
   1511 def length(self):
   1512     """
   1513     Return the number of basis elements whose coefficients in
   1514     ``self`` are nonzero.
   (...)
   1529         4
   1530     """
-> 1531     return len(self)

TypeError: object of type 'sage.matrix.matrix_rational_dense.Matrix_rational_dense' has no len()
sage: matrix(QQ, 4).__len__
<bound method ModulesWithBasis.ElementMethods.__len__ of [0 0 0 0]
[0 0 0 0]
[0 0 0 0]
[0 0 0 0]>

[mkoeppe]

Dependencies: #34509

[mkoeppe]

Changed branch from u/gh-mwageringel/29218 to u/mkoeppe/29218