sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.19k stars 411 forks source link

move methods on general set partitions into AbstractSetPartition #25451

Open mantepse opened 6 years ago

mantepse commented 6 years ago

Several methods currently in SetPartition only make sense for set partitions of {1,...,n}.

As a first step towards resolving this, this tickets moves all the other methods into AbstractSetPartition.

It is not clear to me yet, however, how to proceed beyond that.

CC: @sagetrac-sage-combinat @tscrim @darijgr @alauve @zabrocki

Component: combinatorics

Author: Martin Rubey

Branch/Commit: u/mantepse/move_methods_on_general_set_partitions_into_abstractsetpartition @ 29518ce

Issue created by migration from https://trac.sagemath.org/ticket/25451

mantepse commented 6 years ago

Description changed:

--- 
+++ 
@@ -1 +1,5 @@
+Several methods currently in `SetPartition` only make sense for set partitions of {1,...,n}.

+As a first step towards resolving this, this tickets moves all the other methods into `AbstractSetPartition`.
+
+It is not clear to me yet, however, how to proceed beyond that.
mantepse commented 6 years ago

Author: Martin Rubey

mantepse commented 6 years ago

Branch: u/mantepse/move_methods_on_general_set_partitions_into_abstractsetpartition

tscrim commented 6 years ago

Commit: 29518ce

tscrim commented 6 years ago

New commits:

29518cemove methods into AbstractSetPartition class
d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 6 years ago
comment:4

For every method in AbstractSetPartition I think that there should be a doc test for elements of PartitionDiagram (since PartitionDiagram inherits from AbstractSetPartition). In particular, strict_coarsenings raises an error, but I've been meaning to go through each of the methods and add a doc test because I know that there are others and making changes to this class will inadvertently introduce additional bugs.

mantepse commented 6 years ago
comment:5

Replying to @zabrocki:

In particular, strict_coarsenings raises an error

The problem is simply that this (and other methods) of AbstractSetPartition expect self to be a ClonableArray of Sets. This is enforced in SetPartition.__init__.

I think one question is what the purpose of AbstractSetPartition really is. It now seems to me that it was intended as a class that allows a different representation of set partitions (eg., as a list of tuples, as in PartitionDiagram).

I think it would make sense to have one class for set partitions of {1,...,n}, and another one for general set partitions. The same goes for perfect matchings, permutations, etc. But maybe that's not really useful.

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 6 years ago
comment:6

I think one question is what the purpose of AbstractSetPartition really is. It now seems to me that it was intended as a class that allows a different representation of set partitions (eg., as a list of tuples, as in PartitionDiagram).

AbstractSetPartition is supposed to contain methods that are common to SetPartition and diagrams from diagram algebras (e.g. PartitionDiagrams, BrauerDiagrams, etc.) so that we are not copy/pasting code. Before #25146, diagrams were inheriting all the methods from SetPartition and some of them were not appropriate.