Opened 5 years ago

Closed 4 years ago

#25642 closed defect (fixed)

wrong cardinality in PartitionDiagrams

Reported by: mantepse Owned by:
Priority: major Milestone: sage-8.4
Component: combinatorics Keywords:
Cc: alauve, tscrim, zabrocki Merged in:
Authors: Martin Rubey Reviewers: Erik Bray, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: f54b339 (Commits, GitHub, GitLab) Commit: f54b339d87f9d1ac39dfc95512835c41b518c533
Dependencies: #25462 Stopgaps:

Status badges

Description

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

Change History (19)

comment:1 Changed 5 years ago by mantepse

Branch: u/mantepse/wrong_cardinality_in_partitiondiagrams

comment:2 Changed 5 years ago by mantepse

Authors: Martin Rubey
Commit: 9630c5a6015e6ccf8b0e2824d79031b553c17cc3
Dependencies: #25462
Status: newneeds_review

New commits:

9630c5afix cardinality for PartitionDiagrams, BrauerDiagrams, TemperleyLiebDiagrams and PlanarDiagrams, improve doctests

comment:3 Changed 5 years ago by 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...

comment:4 Changed 5 years ago by 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.

comment:5 Changed 5 years ago by zabrocki

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

comment:6 in reply to:  4 Changed 5 years ago by 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!

comment:7 in reply to:  5 Changed 5 years ago by 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.

comment:8 Changed 4 years ago by embray

Milestone: sage-8.3sage-8.4

comment:9 Changed 4 years ago by 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.

comment:10 Changed 4 years ago by embray

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

comment:11 Changed 4 years ago by embray

Branch: u/mantepse/wrong_cardinality_in_partitiondiagramsu/embray/ticket-25642
Commit: 9630c5a6015e6ccf8b0e2824d79031b553c17cc39835998a9d1a64792ec6a0b70fd8abb66eeb1d6f

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


Last 10 new commits:

ce2962brestore richer doc tests
ba08ff3reviewer's comments
95ce20fprovide iterators which return lists of lists
d2e0e6einline a computation by reviewer's request
947233cMerge branch 'public/25462' of git://trac.sagemath.org/sage into public/combinat/speedup_set_partitions-25462
a79e302Reverted to an_element() and added some additional reviewer changes.
87bf535Cythonizing iterator.
ba6a115Fixing doctests due to ordering change.
321c1b1fix cardinality for PartitionDiagrams, BrauerDiagrams, TemperleyLiebDiagrams and PlanarDiagrams, improve doctests
9835998fix the output of these tests to be consistent with #25462

comment:12 Changed 4 years ago by mantepse

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

Last edited 4 years ago by mantepse (previous) (diff)

comment:13 Changed 4 years ago by tscrim

Reviewers: Erik Bray, Travis Scrimshaw

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.

Last edited 4 years ago by tscrim (previous) (diff)

comment:14 Changed 4 years ago by embray

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

comment:15 Changed 4 years ago by mantepse

Branch: u/embray/ticket-25642u/mantepse/ticket-25642

comment:16 in reply to:  14 Changed 4 years ago by mantepse

Commit: 9835998a9d1a64792ec6a0b70fd8abb66eeb1d6ff54b339d87f9d1ac39dfc95512835c41b518c533

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:

f54b339shorter error message per reviewer's request

comment:17 Changed 4 years ago by tscrim

Status: needs_reviewpositive_review

Thank you.

comment:18 Changed 4 years ago by mantepse

Wow, that was quick!

comment:19 Changed 4 years ago by vbraun

Branch: u/mantepse/ticket-25642f54b339d87f9d1ac39dfc95512835c41b518c533
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.