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

Status badges

Description (last modified by boussica)

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 boussica

  • Authors set to boussica
  • 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 boussica

  • Branch set to u/boussica/add_methods_in_trees

comment:3 Changed 8 years ago by kcrisman

  • Authors changed from boussica to Adrien Boussicault
  • Cc ncohen added
  • Commit set to 6bde7929af3de9ef187c336108a21aec9666aad7

Author, please put your real name :-)

comment:4 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:5 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:6 Changed 6 years ago by chapoton

Adrien, is this "needs_review" ?

comment:7 Changed 6 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-6.9

comment:8 Changed 6 years ago by chapoton

  • Status changed from new to needs_review

comment:9 Changed 6 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_work

Hi,

  1. Please remove trailing whitespaces.
  1. Is it pep8 to have paranthesis after opening paranthesis and before closing ones? (like ( a ))
  1. In
    Return the number of left nodes number is ``direction``
    

the is should be if.

  1. 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 chapoton

  • Branch changed from u/boussica/add_methods_in_trees to public/ticket/16204
  • Commit changed from 6bde7929af3de9ef187c336108a21aec9666aad7 to c250905197bcd4378a01f74c587787da4efcac4d

New commits:

c85fa15Merge branch 'u/boussica/add_methods_in_trees' into 6.9.b6
c250905trac #16204 code formatting

comment:11 Changed 6 years ago by git

  • Commit changed from c250905197bcd4378a01f74c587787da4efcac4d to 7711baf97d9c35d4d28febef8bcb4d757c6256b0

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

7711bafMerge branch 'master' into ticket/16204

comment:12 follow-up: Changed 6 years ago by chapoton

  • Milestone changed from sage-6.9 to sage-6.10

Hello Adrien, in my humble opinion, merging the other way round is better.. Anyway..

comment:13 Changed 6 years ago by chapoton

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 ; follow-up: Changed 6 years ago by boussica

  • 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 boussica

  • Description modified (diff)

Replying to boussica:

Replying to chapoton:

Hello Adrien, in my humble opinion, merging the other way round is better.. Anyway..

Yes, i make an error.

comment:16 Changed 6 years ago by boussica

  • 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 chapoton

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 git

  • Commit changed from 65d12ff04faf1bc92f368a448aa8747f7e4a9083 to 3957b907b6038fc5cdf895f526c73de000c9452d

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

3957b90Remove get_node() from abstract tree

comment:19 Changed 6 years ago by chapoton

Good. But could you please also answer with sentences explaining what you did and what you did not (and why) ?

Last edited 6 years ago by chapoton (previous) (diff)

comment:20 Changed 6 years ago by chapoton

could you please take care of comment:13, namely introduce only one new method in binary_trees.py ?

comment:21 follow-up: Changed 6 years ago by chapoton

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 not-so-useful new functions in binary_trees.py from your branch.

comment:22 in reply to: ↑ 21 Changed 6 years ago by boussica

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 sub-tree.

Replying to chapoton:

bt.left_node_number and bt.right_node_number are already easily available as

bt[0].node_number()

comment:23 Changed 6 years ago by chapoton

What is then a right vertex ? You should explain that in the doc, as precisely as possible.

comment:24 Changed 6 years ago by boussica

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() ?

Last edited 6 years ago by boussica (previous) (diff)

comment:25 Changed 6 years ago by chapoton

  • Cc ncohen removed
  • Keywords tree added
  • Milestone changed from sage-6.10 to sage-7.2

comment:26 Changed 5 years ago by chapoton

node_paths is already available under the name paths

comment:27 Changed 5 years ago by chapoton

  • 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:

bbe19eeMerge branch 'u/boussica/add_some_methods_in_trees' into 7.3.b2
3a22bb8trac 16204 fusing all three methods of binary trees into just one
9178ddbtrac 16204 first step of work on ordered_trees methods

comment:28 Changed 5 years ago by git

  • Commit changed from 9178ddbac96f91bcc7d7d0c2de01edc1cf5ee182 to 636377199bf217146f4ab07dfb2a9d98cdd44553

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

6363771trac 1620' more name changing, less long tests, absolute imports

comment:29 Changed 5 years ago by git

  • Commit changed from 636377199bf217146f4ab07dfb2a9d98cdd44553 to b56069ba8492cb420f93daffdc9a3e9ff2de45cb

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

b56069btrac 1620' minor details

comment:30 Changed 5 years ago by git

  • Commit changed from b56069ba8492cb420f93daffdc9a3e9ff2de45cb to 5ae5076433cd7f8b54fd3fc0275eaf1780e77d5f

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

5ae5076trac 1620' minor details in binary trees

comment:31 Changed 5 years ago by git

  • Commit changed from 5ae5076433cd7f8b54fd3fc0275eaf1780e77d5f to 7e7ece2d71ddaec5f8cfb1cf755e98e853fdf48a

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

7e7ece2trac 16204 more minor details in abstract trees

comment:32 Changed 5 years ago by git

  • Commit changed from 7e7ece2d71ddaec5f8cfb1cf755e98e853fdf48a to e3e52346a1a9d167b5c125dc767fb1aac5be7c5a

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

e3e5234trac 16204 make sure depth starts at 0

comment:33 Changed 5 years ago by git

  • Commit changed from e3e52346a1a9d167b5c125dc767fb1aac5be7c5a to 0c753e82f66870153dbad745859a235c0b0a7734

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

0c753e8trac 16204 final details, plus some pep8 changes in binary_trees

comment:34 Changed 5 years ago by chapoton

  • Authors changed from Adrien Boussicault to Adrien Boussicault, Frédéric Chapoton
  • 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 chapoton

ping ? Adrien ?

comment:36 Changed 5 years ago by chapoton

  • Milestone changed from sage-7.2 to sage-7.3

comment:37 Changed 5 years ago by git

  • Commit changed from 0c753e82f66870153dbad745859a235c0b0a7734 to 0eb710d198b714d6a06dccd84f0d32eb718f3897

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

a5f9574Merge branch 'public/16204' into 7.3.b5
0eb710dtrac 16204 some more doc, plus code details

comment:38 Changed 5 years ago by boussica

  • Status changed from needs_review to positive_review

comment:39 Changed 5 years ago by tscrim

  • Keywords days78 added

comment:40 Changed 5 years ago by vbraun

  • Branch changed from public/16204 to 0eb710d198b714d6a06dccd84f0d32eb718f3897
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.