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: |
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
Branch: | → u/mantepse/wrong_cardinality_in_partitiondiagrams |
---|
comment:2 Changed 5 years ago by
Authors: | → Martin Rubey |
---|---|
Commit: | → 9630c5a6015e6ccf8b0e2824d79031b553c17cc3 |
Dependencies: | → #25462 |
Status: | new → needs_review |
comment:3 Changed 5 years ago by
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 follow-up: 6 Changed 5 years ago by
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 follow-up: 7 Changed 5 years ago by
I'm getting doctest failures on lines 1229 and 1587 because of the order of the output.
comment:6 Changed 5 years ago by
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 beforefoo.list()
, so it does not setcardinality
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 Changed 5 years ago by
comment:8 Changed 4 years ago by
Milestone: | sage-8.3 → sage-8.4 |
---|
comment:9 Changed 4 years ago by
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
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
Branch: | u/mantepse/wrong_cardinality_in_partitiondiagrams → u/embray/ticket-25642 |
---|---|
Commit: | 9630c5a6015e6ccf8b0e2824d79031b553c17cc3 → 9835998a9d1a64792ec6a0b70fd8abb66eeb1d6f |
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
|
comment:13 Changed 4 years ago by
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.
comment:14 follow-up: 16 Changed 4 years ago by
I'm a little confused--should this also be tweaked to merge cleanly with #25662?
comment:15 Changed 4 years ago by
Branch: | u/embray/ticket-25642 → u/mantepse/ticket-25642 |
---|
comment:16 Changed 4 years ago by
Commit: | 9835998a9d1a64792ec6a0b70fd8abb66eeb1d6f → f54b339d87f9d1ac39dfc95512835c41b518c533 |
---|
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
|
comment:19 Changed 4 years ago by
Branch: | u/mantepse/ticket-25642 → f54b339d87f9d1ac39dfc95512835c41b518c533 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
New commits:
fix cardinality for PartitionDiagrams, BrauerDiagrams, TemperleyLiebDiagrams and PlanarDiagrams, improve doctests