Removing __len__ for combinatorial classes (depend on #5200,#4549)

[hivert]

After some discussion on sage-combinat-devel, it was decided that __len__ should no be used to get the size of a combinatorial class. Indeed the specifications of __len__ says that it has to return a plain python int, whereas we want to deal with huge and even infinite sets. Unfortunately due to __len__ being called by list / filter / map and maybe some other functions, it is not possible to issue a warning. So it was decided to simply remove it and issue an error, telling to call .cardinality() instead. The former usage of .count() is also deprecated.

Combinat has to be adapted to this and also to the deprecation of iterator in favor of __iter__

I'm preparing a patch to solve those issues.

Cheers,

Florent

CC: @sagetrac-sage-combinat

Component: combinatorics

Keywords: len, count, combinatorial class

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

[hivert]

Description changed:

--- 
+++ 
@@ -1,3 +1,11 @@
-After some discussion on sage-combinat-devel, it was decided that `__len__` should no be used to get the size of a combinatorial class because. Indeed the specifications of `__len__` says that it has to return a plain python int, whereas we want to deal with huge and even infinite sets. Unfortunately due to `__len__` being called by list / filter / map and maybe some other functions, it is not possible to issue a warning. So it was decided to simply remove it and issue an error, telling to call .count() instead. 
+After some discussion on sage-combinat-devel, it was decided that `__len__` should no be used to get the size of a combinatorial class. Indeed the specifications of `__len__` says that it has to return a plain python int, whereas we want to deal with huge and even infinite sets. Unfortunately due to `__len__` being called by `list / filter / map` and maybe some other functions, it is not possible to issue a warning. So it was decided to simply remove it and issue an error, telling to call `.cardinality()` instead. The former usage of `.count()` is also deprecated.
 
-Some part of combinat has to be fixed accordingly. 
+Combinat has to be adapted to this and also to the deprecation of `iterator` in favor of `__iter__`
+
+I'm preparing a patch to solve those issues. 
+
+Cheers,
+
+Florent
+ 
+

[hivert]

comment:3

I've attached a patch for the cleanup. It should apply cleanly on top of #5200.

Author: Florent Hivert

[hivert]

Attachment: combinatorialclass_iter_len_count_cleanup.patch.gz

Good version of the file

[nthiery]

comment:5

When applying the patch on top of 3.4 with the following patches applied:

zephyr-/opt/sage/devel/sage>hg qapplied
integer_lists_lex-4549-submitted.patch
subsets_subwords-5200-submitted.2.patch
subsets_subwords-5200-review.patch
subwords_fix-5534-submitted.patch
combinatorialclass_iter_len_count_cleanup.patch

I get a reject in

zephyr-/opt/sage/devel/sage>cat sage/combinat/subset.py.rej
--- subset.py
+++ subset.py
@@ -522,7 +522,7 @@
             [0, 1, 3]
             sage: S._multiplicities
             [1, 2, 1]
-            sage: Subsets([1,2,3,3], multiset=True).count()
+            sage: Subsets([1,2,3,3], multiset=True).cardinality()
             12
             sage: S == loads(dumps(S))
             True
@@ -604,7 +604,7 @@
             sage: S = Subsets([1,2,3,3],2, multiset=True)
             sage: S._k
             2
-            sage: S.count()
+            sage: S.cardinality()
             4
             sage: S.first()
             [1, 2]

Moreover, the following tests get broken:

        sage -t  "3.4.rc0/devel/sage-patches/sage/combinat/sf/ns_macdonald.py"
        sage -t  "3.4.rc0/devel/sage-patches/sage/combinat/subset.py"
        sage -t  "3.4.rc0/devel/sage-patches/sage/combinat/words/words.py"

either by the deprecation warnings, or for words by a change of output:
96889010407L -> 96889010407

Other than this, the patch sounds good to me!

[nthiery]

comment:6

In CartesianProduct, shouldn't we test first if it.cardinality works, and resort to len if this raised an AttributeError?
Line 3 of the doc of InfiniteAbstractCombinatorialClass:

• inerit -> inherit, wich -> which, count -> cardinality, ...
  The log of the patch should advertise this new class
  In CyclicPermutations: leave at least a note that this deviates from the standard (iter/list take no argument), and should be cleaned up further at some point
  In the examples (like in SemistandardTableaux), do we want to use: [t for t in iterable] or list(iterable)

[saliola]

comment:7

I've looked over the changes to the words files. Everything looks good. To fix the problem with the doctests related to 96889010407L that nthiery mentioned above, just have Word_n.cardinality() (in sage/combinat/words/words.py) return a Sage Integer instead of a Python int. If you do this, then it also deals with #4938.

[hivert]

Addressed Nicolas reqests.