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)).

Depends on #25462

CC: @alauve @tscrim @zabrocki

Component: combinatorics

Author: Martin Rubey

Branch/Commit: f54b339

Reviewer: Erik Bray, Travis Scrimshaw

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

[mantepse]

Branch: u/mantepse/wrong_cardinality_in_partitiondiagrams

[mantepse]

Dependencies: #25462

[mantepse]

Commit: 9630c5a

[mantepse]

New commits:

9630c5a

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

[mantepse]

Author: Martin Rubey

[mantepse]

comment:3

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]

comment:4

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]

comment:5

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

[mantepse]

comment:6

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]

comment:7

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]

comment:9

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]

comment:10

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

[embray]

Changed branch from u/mantepse/wrong_cardinality_in_partitiondiagrams to u/embray/ticket-25642

[embray]

comment:11

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

[embray]

Changed commit from 9630c5a to 9835998

[mantepse]

comment:12

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

[tscrim]

Reviewer: Erik Bray, Travis Scrimshaw

[tscrim]

comment:13

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]

comment:14

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

[mantepse]

Changed branch from u/embray/ticket-25642 to u/mantepse/ticket-25642

[mantepse]

comment:16

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

[mantepse]

Changed commit from 9835998 to f54b339

[tscrim]

comment:17

Thank you.

[mantepse]

comment:18

Wow, that was quick!

[vbraun]

Changed branch from u/mantepse/ticket-25642 to f54b339