Opened 8 years ago
Closed 5 years ago
#16204 closed enhancement (fixed)
Add some methods in trees
Reported by:  boussica  Owned by:  

Priority:  minor  Milestone:  sage7.3 
Component:  combinatorics  Keywords:  tree, days78 
Cc:  Merged in:  
Authors:  Adrien Boussicault, Frédéric Chapoton  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  0eb710d (Commits, GitHub, GitLab)  Commit:  0eb710d198b714d6a06dccd84f0d32eb718f3897 
Dependencies:  Stopgaps: 
Description (last modified by )
OK, i clean the the branch by making a new branch from Sage 6.10 with all the minimal modifications.
Change History (40)
comment:1 Changed 8 years ago by
 Component changed from PLEASE CHANGE to combinatorics
 Priority changed from major to minor
 Type changed from PLEASE CHANGE to enhancement
comment:2 Changed 8 years ago by
 Branch set to u/boussica/add_methods_in_trees
comment:3 Changed 8 years ago by
 Cc ncohen added
 Commit set to 6bde7929af3de9ef187c336108a21aec9666aad7
comment:4 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:5 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:6 Changed 6 years ago by
Adrien, is this "needs_review" ?
comment:7 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.9
comment:8 Changed 6 years ago by
 Status changed from new to needs_review
comment:9 Changed 6 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to needs_work
Hi,
 Please remove trailing whitespaces.
 Is it pep8 to have paranthesis after opening paranthesis and before closing ones? (like
( a )
)
 In
Return the number of left nodes number is ``direction``
the is
should be if
.
 Using recursive function is a bad idea. Python is not designed to support smartly recursive function calls. You would better use depth first search.
Vincent
comment:10 Changed 6 years ago by
 Branch changed from u/boussica/add_methods_in_trees to public/ticket/16204
 Commit changed from 6bde7929af3de9ef187c336108a21aec9666aad7 to c250905197bcd4378a01f74c587787da4efcac4d
comment:11 Changed 6 years ago by
 Commit changed from c250905197bcd4378a01f74c587787da4efcac4d to 7711baf97d9c35d4d28febef8bcb4d757c6256b0
Branch pushed to git repo; I updated commit sha1. New commits:
7711baf  Merge branch 'master' into ticket/16204

comment:12 followup: ↓ 14 Changed 6 years ago by
 Milestone changed from sage6.9 to sage6.10
Hello Adrien, in my humble opinion, merging the other way round is better.. Anyway..
comment:13 Changed 6 years ago by
in binary tree
, please add just one function
the other two are just alias, and not needed. (avoid namespace pollution)
and please define what you mean by right and left node, as this is not clear at all !
comment:14 in reply to: ↑ 12 ; followup: ↓ 15 Changed 6 years ago by
 Description modified (diff)
Replying to chapoton:
Hello Adrien, in my humble opinion, merging the other way round is better.. Anyway..
comment:15 in reply to: ↑ 14 Changed 6 years ago by
 Description modified (diff)
comment:16 Changed 6 years ago by
 Branch changed from public/ticket/16204 to u/boussica/add_some_methods_in_trees
 Commit changed from 7711baf97d9c35d4d28febef8bcb4d757c6256b0 to 65d12ff04faf1bc92f368a448aa8747f7e4a9083
 Description modified (diff)
comment:17 Changed 6 years ago by
You have not answered my last comment, where I suggest to add just one function instead of three in the binary_tree
file.
The command get_node
is already available as:
sage: sage: T = OrderedTree([[[], [[], [[]]]], [], [[[], []]], [], []]) sage: T[()] ______o_______ / / / / / _o__ o o o o / /  o o_ o_ / / / / o o o o  o sage: T ______o_______ / / / / / _o__ o o o o / /  o o_ o_ / / / / o o o o  o sage: T[(0,1)] o_ / / o o  o
So please remove it.
comment:18 Changed 6 years ago by
 Commit changed from 65d12ff04faf1bc92f368a448aa8747f7e4a9083 to 3957b907b6038fc5cdf895f526c73de000c9452d
Branch pushed to git repo; I updated commit sha1. New commits:
3957b90  Remove get_node() from abstract tree

comment:19 Changed 6 years ago by
Good. But could you please also answer with sentences explaining what you did and what you did not (and why) ?
comment:20 Changed 6 years ago by
could you please take care of comment:13, namely introduce only one new method in binary_trees.py ?
comment:21 followup: ↓ 22 Changed 6 years ago by
bt.left_node_number
and bt.right_node_number
are already easily available as
bt[0].node_number()
and
bt[1].node_number()
so please remove the 3 notsouseful new functions in binary_trees.py
from your branch.
comment:22 in reply to: ↑ 21 Changed 6 years ago by
Hi,
No, it is not the same function.
bt.right_node_number
gives the number of right vertices inside the tree.
It is not the number of vertices inside the right subtree.
Replying to chapoton:
bt.left_node_number
andbt.right_node_number
are already easily available asbt[0].node_number()
comment:23 Changed 6 years ago by
What is then a right vertex ? You should explain that in the doc, as precisely as possible.
comment:24 Changed 6 years ago by
In a binary tree, there are right children and left children. I use also the term of right vertices and left vertices. Perhaps, it is better i use bt.right_child_number() and left_child_number() ?
comment:25 Changed 6 years ago by
 Cc ncohen removed
 Keywords tree added
 Milestone changed from sage6.10 to sage7.2
comment:26 Changed 5 years ago by
node_paths
is already available under the name paths
comment:27 Changed 5 years ago by
 Branch changed from u/boussica/add_some_methods_in_trees to public/16204
 Commit changed from 3957b907b6038fc5cdf895f526c73de000c9452d to 9178ddbac96f91bcc7d7d0c2de01edc1cf5ee182
I have started the job, not yet done.
@boussica, do you need all these methods in ordered_trees ? Some of them seem not to be of general use, and I would prefer to avoid them.
New commits:
bbe19ee  Merge branch 'u/boussica/add_some_methods_in_trees' into 7.3.b2

3a22bb8  trac 16204 fusing all three methods of binary trees into just one

9178ddb  trac 16204 first step of work on ordered_trees methods

comment:28 Changed 5 years ago by
 Commit changed from 9178ddbac96f91bcc7d7d0c2de01edc1cf5ee182 to 636377199bf217146f4ab07dfb2a9d98cdd44553
Branch pushed to git repo; I updated commit sha1. New commits:
6363771  trac 1620' more name changing, less long tests, absolute imports

comment:29 Changed 5 years ago by
 Commit changed from 636377199bf217146f4ab07dfb2a9d98cdd44553 to b56069ba8492cb420f93daffdc9a3e9ff2de45cb
Branch pushed to git repo; I updated commit sha1. New commits:
b56069b  trac 1620' minor details

comment:30 Changed 5 years ago by
 Commit changed from b56069ba8492cb420f93daffdc9a3e9ff2de45cb to 5ae5076433cd7f8b54fd3fc0275eaf1780e77d5f
Branch pushed to git repo; I updated commit sha1. New commits:
5ae5076  trac 1620' minor details in binary trees

comment:31 Changed 5 years ago by
 Commit changed from 5ae5076433cd7f8b54fd3fc0275eaf1780e77d5f to 7e7ece2d71ddaec5f8cfb1cf755e98e853fdf48a
Branch pushed to git repo; I updated commit sha1. New commits:
7e7ece2  trac 16204 more minor details in abstract trees

comment:32 Changed 5 years ago by
 Commit changed from 7e7ece2d71ddaec5f8cfb1cf755e98e853fdf48a to e3e52346a1a9d167b5c125dc767fb1aac5be7c5a
Branch pushed to git repo; I updated commit sha1. New commits:
e3e5234  trac 16204 make sure depth starts at 0

comment:33 Changed 5 years ago by
 Commit changed from e3e52346a1a9d167b5c125dc767fb1aac5be7c5a to 0c753e82f66870153dbad745859a235c0b0a7734
Branch pushed to git repo; I updated commit sha1. New commits:
0c753e8  trac 16204 final details, plus some pep8 changes in binary_trees

comment:34 Changed 5 years ago by
 Status changed from needs_work to needs_review
ok, this is ready for review.
I used the opportunity to do a few pep8 changes in binary_trees.
Adrien, please confirm that you are ok with the code and doc.
comment:35 Changed 5 years ago by
ping ? Adrien ?
comment:36 Changed 5 years ago by
 Milestone changed from sage7.2 to sage7.3
comment:37 Changed 5 years ago by
 Commit changed from 0c753e82f66870153dbad745859a235c0b0a7734 to 0eb710d198b714d6a06dccd84f0d32eb718f3897
comment:38 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:39 Changed 5 years ago by
 Keywords days78 added
comment:40 Changed 5 years ago by
 Branch changed from public/16204 to 0eb710d198b714d6a06dccd84f0d32eb718f3897
 Resolution set to fixed
 Status changed from positive_review to closed
Author, please put your real name :)