wrong cardinality in PartitionDiagrams

[mantepse]

There is a simple bug and a strange behaviour:

sage: import sage.combinat.diagram_algebras as da
sage: pd = da.PartitionDiagrams(2.5)
sage: pd.cardinality()
15
sage: len(list(pd))
52
sage: pd.cardinality()
15
sage: len(pd.list())
52
sage: pd.cardinality()
52

The implementation of PartitionDiagrams.cardinality is a trivial mistake, and easy to fix. It currently reads:

        if self.order in ZZ:
            return bell_number(2*self.order)
        return bell_number(2*(self.order-1/2))

but should be

        return bell_number(2*self.order)

However, I do not understand why, after invoking PartitionDiagrams(2.5).list(), the method from finite_enumerated_sets is used, and I do not understand why this is not the case when invoking list(PartitionDiagrams(2.5)).

[mantepse]

New commits:

​9630c5a

fix cardinality for PartitionDiagrams, BrauerDiagrams, TemperleyLiebDiagrams and PlanarDiagrams, improve doctests

[mantepse]

I do not know whether the strange behaviour observed in the description is something that needs fixing, nor how to do it, if so...

[tscrim]

Why does this depend on #25462?

list(foo) calls the iterator first before foo.list(), so it does not set cardinality to _cardinality_from_list. This is the correct behavior and nothing strange about it. If there was no bug, you would never notice the difference.

[zabrocki]

I'm getting doctest failures on lines 1229 and 1587 because of the order of the output.

[mantepse]

Replying to tscrim:

Why does this depend on #25462?

Because after #25462 the output order changes, and there is no point in changing the doctests twice. But if you wish, I can do this, too.

list(foo) calls the iterator first before foo.list(), so it does not set cardinality to _cardinality_from_list. This is the correct behavior and nothing strange about it. If there was no bug, you would never notice the difference.

OK, great!

[mantepse]

Replying to zabrocki:

I'm getting doctest failures on lines 1229 and 1587 because of the order of the output.

Yes, I do not know how to make this properly depend on #25462. If you do pull in #25462, the output order will be correct.

[embray]

I noticed this problem in the tests on Python 3, where the problem becomes more pronounced due to slightly different code paths covered by the tests.

[embray]

Since this depends on #25462, I'll rebase your branch on top of it. The rest looks like it makes sense to me.

[embray]

I rebased this on top of #25462, but I still had to update a couple of the doctests.

---

Last 10 new commits:

​ce2962b	restore richer doc tests
​ba08ff3	reviewer's comments
​95ce20f	provide iterators which return lists of lists
​d2e0e6e	inline a computation by reviewer's request
​947233c	Merge branch 'public/25462' of git://trac.sagemath.org/sage into public/combinat/speedup_set_partitions-25462
​a79e302	Reverted to an_element() and added some additional reviewer changes.
​87bf535	Cythonizing iterator.
​ba6a115	Fixing doctests due to ordering change.
​321c1b1	fix cardinality for PartitionDiagrams, BrauerDiagrams, TemperleyLiebDiagrams and PlanarDiagrams, improve doctests
​9835998	fix the output of these tests to be consistent with #25462

[mantepse]

Warning: please note also #25662... (are you touching contains?)

[tscrim]

Martin, you were the one who tweaked the __contains__.

Something closer to bikeshedding, but I would want changed before a positive review is splitting the lines for the NotImplementedError messages (to ~80 char/line) and making them more concise:

-            raise NotImplementedError("from_involution_permutation_triple is only implemented for Brauer diagrams of integer order, not for order %s" %(self.order))
+            raise NotImplementedError("only implemented for integer order,"
+                                      " not for order %s" % (self.order))

and same for symmetric_diagrams. Similar for the output in the doctests.

[embray]

I'm a little confused--should this also be tweaked to merge cleanly with #25662?

[mantepse]

Replying to embray:

I'm a little confused--should this also be tweaked to merge cleanly with #25662?

No, if I recall correctly, I made this ticket a dependency of #25662. So this ticket should go in first, then #25659 (which is independent of this ticket), and finally #25662.

---

New commits:

​f54b339

shorter error message per reviewer's request

[tscrim]

Thank you.

[mantepse]

Wow, that was quick!