sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.36k stars 462 forks source link

Combinatorial m-ary trees #13987

Open VivianePons opened 11 years ago

VivianePons commented 11 years ago

In the same manner as #8703 we want to implement trees with a fixed number of subtrees (equivalent of binary trees but with m subtrees). Especially to use them in the context of the m-Tamari lattices.

This ticket depends on #8703 as we use the implementation of OrderedTrees.

Related:

CC: @hivert @sagetrac-sage-combinat @tscrim @darijgr

Component: combinatorics

Keywords: trees

Author: Viviane Pons

Branch/Commit: public/new-13987 @ a2d92f4

Reviewer: Darij Grinberg

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

darijgr commented 6 years ago
comment:41

OK, what about point (2)?

And I'm fine with no inheritance; it's enough that the constructor is working.

tscrim commented 6 years ago
comment:42

Replying to @fchapoton:

I did some of the simple suggested changes.

Thank you.

I would rather not have any inheritance between BinaryTrees and MAryTrees. Conversion is enough.

Why not? It doesn't make sense to me both mathematically and programmatically. I can understand you wanting to postpone it for a later ticket (although I think change it really quickly), but I don't understand the opposition to having inheritance (and the constructors returning the BinaryTree* object when m = 2). Am I missing something about the structures of these classes?

embray commented 5 years ago
comment:44

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

fchapoton commented 5 years ago

Changed branch from public/combinat/13987-mary-trees to public/new-13987

fchapoton commented 5 years ago
comment:45

I have made a fresh new branch with a single commit, as the previous one was polluted by many merge commits.


New commits:

eb12c03m-ary trees
fchapoton commented 5 years ago

Changed commit from 153b2d7 to eb12c03

embray commented 4 years ago
comment:46

Ticket retargeted after milestone closed

mkoeppe commented 4 years ago
comment:47

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

2818096Merge branch 'public/new-13987' in 9.2.b5
a2d92f4fixes for python3 and dropped SearchForest
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from eb12c03 to a2d92f4

mkoeppe commented 4 years ago
comment:49

What's the status of the review on this ticket?

darijgr commented 4 years ago
comment:50

I don't remember more than is said in the comments above, and I'm not going to have time for this in the foreseeable time... sorry, not very useful I know.

mkoeppe commented 3 years ago
comment:52

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

mkoeppe commented 3 years ago
comment:53

Setting a new milestone for this ticket based on a cursory review.

1adc0eef-8957-46d9-975b-2dd71dfbd9ba commented 3 years ago
comment:54

I'd like to try and help get this finished, though I'm not sure I'm qualified to be final judge and jury on the OOP/MRO aspects. At the very least, I can implement Travis's suggestion of removing LabelledMAryTrees.__call__ , and try to reverse the order of inheritance for class MAryTrees_all(DisjointUnionEnumeratedSets, MAryTrees) and remove the `__call__ and make sure nothing breaks.

In terms of m=2, people seem fine with having conversion for now, and having inheritance be part of a later ticket to get this finished.

I can implement Travis's suggestion that the documentation being clearer about these being ordered/plane trees.

In terms of check only checking the outer layer, I'm pretty sure this should be fine, and it will recursively check the entire tree. But I can try and make a test case to be sure.

I'll make a thorough pass in the next day or two to test things out, check doc and examples for clarity to somebody that may not be as familiar with m-ary trees, and make sure the doc compiles properly. One thing that's more a note to self to fix later when I do this is that enumerated m-ary trees of a given size is still labeled as 'enumerated set of binary trees of a given size' in the code.

Also reminder to self to check (1) as part of comment:37.

1adc0eef-8957-46d9-975b-2dd71dfbd9ba commented 3 years ago
comment:55

In terms of class MAryTrees_all(DisjointUnionEnumeratedSets, MAryTrees)versus class MAryTrees_all(MAryTrees, DisjointUnionEnumeratedSets) , the way it is now matches what is done in binary trees.

I can't manage to track down exactly what's happening, but as the doc indicates, the __call__ methods are necessary to make sure that empty input is treated as None instead of getting passed as 0. Maybe something inherited from OrderedTrees and their assumption that there are no empty trees (and thus OrderedTrees()() == OrderedTrees()([])? Switching the MRO above did not resolve the issue.

This fix/hack is also implemented in BinaryTrees, though only for BinaryTrees_all. So BinaryTrees()() == BinaryTrees()(None) == ., but in BinaryTrees_size, BinaryTrees(0)() gives an error about the int 0 not being callable, whereas BinaryTrees(0)(None) ==.

I would be of the opinion that if the way things done here is the way they've been forever in BinaryTrees, then they shouldn't be holding back this ticket from getting a positive review.

slel commented 3 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,8 @@
 In the same manner as #8703 we want to implement trees with a fixed number of subtrees (equivalent of binary trees but with m subtrees). Especially to use them in the context of the m-Tamari lattices.

 This ticket depends on #8703 as we use the implementation of OrderedTrees.
+
+Related:
+
+- #11369: generate k-trees up to symmetry
+- #19479: Implement k-ary trees
mkoeppe commented 2 years ago
comment:57

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.