Opened 5 years ago
Closed 4 years ago
#25642 closed defect (fixed)
wrong cardinality in PartitionDiagrams
Reported by:  mantepse  Owned by:  

Priority:  major  Milestone:  sage8.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.order1/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 followup: 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 followup: 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:  sage8.3 → sage8.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/ticket25642 

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_partitions25462

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 followup: 16 Changed 4 years ago by
I'm a little confusedshould this also be tweaked to merge cleanly with #25662?
comment:15 Changed 4 years ago by
Branch:  u/embray/ticket25642 → u/mantepse/ticket25642 

comment:16 Changed 4 years ago by
Commit:  9835998a9d1a64792ec6a0b70fd8abb66eeb1d6f → f54b339d87f9d1ac39dfc95512835c41b518c533 

Replying to embray:
I'm a little confusedshould 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/ticket25642 → f54b339d87f9d1ac39dfc95512835c41b518c533 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
fix cardinality for PartitionDiagrams, BrauerDiagrams, TemperleyLiebDiagrams and PlanarDiagrams, improve doctests