Add to check for fixed total/length in IntegerVector

[trevorkarn]

The current IntegerVector.check() method only checks for the positivity of each element. This adds a check for specified totals (n) and lengths (k).

This ticket also allows for any collections.abc.Sequence object to be checked as an integer vector.

CC: @tscrim @trevorkarn @darijgr

Component: combinatorics

Keywords: gsoc2022 integer-vector check

Author: Trevor K. Karn

Branch/Commit: 43e6813

Reviewer: Travis Scrimshaw

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

[trevorkarn]

Changed keywords from gsoc2022 to gsoc2022 integer-vector check

[trevorkarn]

comment:2

There is also a defect in this:

sage: IV33 = IntegerVectors(n=3, k=3)
sage: IV33([0])
[0]
sage: IV33([0]) in IV33
True

[trevorkarn]

Branch: u/tkarn/34510

[trevorkarn]

New commits:

c071210

Add tests, remove buggy check, add to check

[trevorkarn]

Commit: c071210

[tscrim]

comment:4

I would just check that it belongs to the parent. The first test should be done by that anyways.

Also, for the error message:

f"{self} is not an element of {self.parent()}"

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:5

Once changed and a green bot, positive review.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9fc9a42	Add tests, remove buggy check, add to check
d5522d2	Fix error message and fix bug and add tests

[sagetrac-git]

Changed commit from c071210 to d5522d2

[trevorkarn]

comment:7

Should be good to go. Removing the first check allowed [-1] to be understood as an IntegerVector so I fixed that in the IntegerVector.__contains__.

[tscrim]

comment:8

Good catch on another bug. [-1] in IntegerVectors() is indeed bad. One last thing is changing

-if not isinstance(x, (list, tuple, IntegerVector)):
+if not isinstance(x, sequence):

using the from collections.abc import sequence (that way it also takes things like Compositions as well). We should just do that here as well (with an update to the ticket description too).

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4ddf379	Add tests, remove buggy check, add to check
43e6813	List/tuple -> sequence

[sagetrac-git]

Changed commit from d5522d2 to 43e6813

[trevorkarn]

comment:10

Replying to Travis Scrimshaw:

One last thing is changing

-if not isinstance(x, (list, tuple, IntegerVector)):
+if not isinstance(x, sequence):

using the from collections.abc import sequence

Done!

[trevorkarn]

comment:13

Here's something surprising! Compositions are not understood as Sequences.

sage: c = Composition([3,4,2,4])
sage: from collections.abc import Sequence
sage: isinstance(c, Sequence)
False

[trevorkarn]

comment:14

Replying to Trevor Karn:

Here's something surprising! Compositions are not understood as Sequences.

sage: c = Composition([3,4,2,4])
sage: from collections.abc import Sequence
sage: isinstance(c, Sequence)
False

The bug because of the Compositions, so I'm opening #34527 and this is now ready for review.

[tscrim]

comment:16

Actually, I suspect this holds for things more general things within Sage because of the extra requirements of a __reversed__() and count() methods. We can weaken this to collection.

[trevorkarn]

comment:17

Replying to Travis Scrimshaw:

Actually, I suspect this holds for things more general things within Sage because of the extra requirements of a __reversed__() and count() methods. We can weaken this to collection.

I think we discussed this offline but I just want to confirm that this should be a Sequence because there is implicitly a fixed order?

[tscrim]

comment:18

Yes, that's correct.

[trevorkarn]

comment:19

Replying to Travis Scrimshaw:

Yes, that's correct.

Ok then I think this ticket is ready for review. Am I missing anything?

[tscrim]

comment:20

LGTM. Thank you.

[vbraun]

Changed branch from u/tkarn/34510 to 43e6813