Opened 2 years ago
Closed 2 years ago
#29634 closed enhancement (fixed)
some details in combinat about partitions
Reported by:  Frédéric Chapoton  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  combinatorics  Keywords:  
Cc:  Travis Scrimshaw  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  d15ce93 (Commits, GitHub, GitLab)  Commit:  d15ce93bbd1280cda827dbc0770b80fbcdc8760e 
Dependencies:  Stopgaps: 
Description (last modified by )
 using multinomial from arith
 some pep8 details
 almost getting rid of six in the 2 modified files
Change History (11)
comment:1 Changed 2 years ago by
Branch:  → u/chapoton/29634 

Commit:  → a08db46dcc85650aafbe690b2d65a4f81d3efcf8 
Status:  new → needs_review 
comment:2 Changed 2 years ago by
Description:  modified (diff) 

comment:4 Changed 2 years ago by
 if parts in Words() or (len(parts) > 0 and (parts[0] in ZZ or isinstance(parts[0], str))): + if parts in Words() or (len(parts) and (parts[0] in ZZ or isinstance(parts[0], str))):
Why only len(parts)
and not simply parts
?
I would do this
return OrderedSetPartitions().from_finite_word(Words()(parts)) else:  P = OrderedSetPartitions( reduce(lambda x,y: x.union(y), map(Set, parts), Set([])) ) + P = OrderedSetPartitions([reduce(lambda x,y: x.union(y), # Maybe x.union(frozenset(y))? + parts, frozenset())) return P.element_class(P, parts)
as I think we currently require the elements to be hashable and we can let the OrderedSetPartitions
constructor handle the rest.
1 on this change:
 par = parent(self) + par = self.parent()
as using the function is actually faster (Cython function versus Python method call).
LGTM modulo these comments.
comment:5 Changed 2 years ago by
Commit:  a08db46dcc85650aafbe690b2d65a4f81d3efcf8 → 6fd708e2a4df5dd83dd08bcd8dfe08b448bed9de 

Branch pushed to git repo; I updated commit sha1. New commits:
6fd708e  trac 29634 some further details

comment:6 Changed 2 years ago by
thanks. Fixed, hopefully, though there is a doubt about frozenset. Let us wait for the patchbot.
comment:7 Changed 2 years ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → positive_review 
Green bot. Thank you.
comment:9 Changed 2 years ago by
Commit:  6fd708e2a4df5dd83dd08bcd8dfe08b448bed9de → d15ce93bbd1280cda827dbc0770b80fbcdc8760e 

Branch pushed to git repo; I updated commit sha1. New commits:
d15ce93  Merge branch 'u/chapoton/29634' with 9.2.b0

comment:10 Changed 2 years ago by
Status:  needs_work → positive_review 

trivial rebase, setting back to positive review
comment:11 Changed 2 years ago by
Branch:  u/chapoton/29634 → d15ce93bbd1280cda827dbc0770b80fbcdc8760e 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
some details in combinat about partitions