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: sage-9.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:

Status badges

Description (last modified by Frédéric Chapoton)

  • 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 Frédéric Chapoton

Branch: u/chapoton/29634
Commit: a08db46dcc85650aafbe690b2d65a4f81d3efcf8
Status: newneeds_review

New commits:

a08db46some details in combinat about partitions

comment:2 Changed 2 years ago by Frédéric Chapoton

Description: modified (diff)

comment:3 Changed 2 years ago by Frédéric Chapoton

Cc: Travis Scrimshaw added

green bot, please review

comment:4 Changed 2 years ago by Travis Scrimshaw

-        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 git

Commit: a08db46dcc85650aafbe690b2d65a4f81d3efcf86fd708e2a4df5dd83dd08bcd8dfe08b448bed9de

Branch pushed to git repo; I updated commit sha1. New commits:

6fd708etrac 29634 some further details

comment:6 Changed 2 years ago by Frédéric Chapoton

thanks. Fixed, hopefully, though there is a doubt about frozenset. Let us wait for the patchbot.

comment:7 Changed 2 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Green bot. Thank you.

comment:8 Changed 2 years ago by Volker Braun

Status: positive_reviewneeds_work

Merge conflict

comment:9 Changed 2 years ago by git

Commit: 6fd708e2a4df5dd83dd08bcd8dfe08b448bed9ded15ce93bbd1280cda827dbc0770b80fbcdc8760e

Branch pushed to git repo; I updated commit sha1. New commits:

d15ce93Merge branch 'u/chapoton/29634' with 9.2.b0

comment:10 Changed 2 years ago by Frédéric Chapoton

Status: needs_workpositive_review

trivial rebase, setting back to positive review

comment:11 Changed 2 years ago by Volker Braun

Branch: u/chapoton/29634d15ce93bbd1280cda827dbc0770b80fbcdc8760e
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.