Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Provide an iterative version on some functions on trees #39519

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Provide an iterative version on some functions on trees #39519

wants to merge 10 commits into from

Conversation

Oscfon
Copy link

@Oscfon Oscfon commented Feb 13, 2025 •
edited
Loading

This pull request provide iterative methods on AbstractTrees which work both on OrderedTree and BinaryTree.
The problem was that some functions like node_number reach the recursion limit on huge instances.

sage: T = OrderedTree([])
sage: for _ in range(10000):
....:     T = OrderedTree([])
sage: T.node_number()
RecursionError

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

@Oscfon Oscfon added sd125 sage days 125 sd128 tickets of Sage Days 128 Le Teich and removed sd125 sage days 125 labels Feb 13, 2025
mantepse
mantepse reviewed Feb 14, 2025
View reviewed changes
src/sage/combinat/abstract_tree.py Outdated
if not subtree.is_empty():
stack.append(subtree)
else:
stack.pop()
node = stack.pop()
action(node)

def contour_traversal(self,first_action=None, middle_action=None, final_action=None, leaf_action=[]):
Copy link
Contributor

@mantepse mantepse Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def contour_traversal(self,first_action=None, middle_action=None, final_action=None, leaf_action=[]):
def contour_traversal(self, first_action=None, middle_action=None, final_action=None, leaf_action=None):

I don't see why leaf_action should not be None by default.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want that if you change only first_action and final_action, leaf_action will be the action of both of them but it's perhaps a bad idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, can you give an example?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at the depth code : i only change the final_action and it change accordingly the leaf_action.
I would like it to work like that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think again at this point and I think you are right, it's probably better that the default leaf_action is None.
I made the change in the last commit.

@fchapoton @Oscfon
Apply suggestions from code review
adcc6ab 
to fix the tests and the linter

Co-authored-by: Oscfon <[email protected]>
@fchapoton
Copy link
Contributor

attention, je me suis permis de faire quelques modifs, il faut faire "git pull" pour les recevoir localement avant de travailler a nouveau

@fchapoton
Update abstract_tree.py
5ba4f02 
last fix for the linter
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 18, 2025 •
edited
Loading

Documentation preview for this PR (built with commit c5835a2; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@fchapoton
Copy link
Contributor

y a trois tests qui ne passent pas

@Oscfon
Copy link
Author

Oscfon commented Feb 21, 2025

J'avais oublié une dépendance. Ça devrait être bon

@fchapoton
Copy link
Contributor

Je viens de proposer trois suggestions.

@fchapoton fchapoton self-assigned this Feb 26, 2025
fchapoton
fchapoton approved these changes Feb 27, 2025
View reviewed changes
Copy link
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, good to go. Merci !

vbraun pushed a commit to vbraun/sage that referenced this pull request Mar 1, 2025
sagemathgh-39519: Provide an iterative version on some functions on t…
86e8333 
…rees

    
This pull request provide iterative methods on AbstractTrees which work
both on OrderedTree and BinaryTree.
The problem was that some functions like node_number reach the recursion
limit on huge instances.
```
sage: T = OrderedTree([])
sage: for _ in range(10000):
....:     T = OrderedTree([])
sage: T.node_number()
RecursionError
```

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#39519
Reported by: Oscfon
Reviewer(s): Frédéric Chapoton, Martin Rubey, Oscfon
vbraun pushed a commit to vbraun/sage that referenced this pull request Mar 1, 2025
sagemathgh-39519: Provide an iterative version on some functions on t…
8cb987d 
…rees

    
This pull request provide iterative methods on AbstractTrees which work
both on OrderedTree and BinaryTree.
The problem was that some functions like node_number reach the recursion
limit on huge instances.
```
sage: T = OrderedTree([])
sage: for _ in range(10000):
....:     T = OrderedTree([])
sage: T.node_number()
RecursionError
```

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#39519
Reported by: Oscfon
Reviewer(s): Frédéric Chapoton, Martin Rubey, Oscfon
vbraun pushed a commit to vbraun/sage that referenced this pull request Mar 2, 2025
sagemathgh-39519: Provide an iterative version on some functions on t…
690cdbb 
…rees

    
This pull request provide iterative methods on AbstractTrees which work
both on OrderedTree and BinaryTree.
The problem was that some functions like node_number reach the recursion
limit on huge instances.
```
sage: T = OrderedTree([])
sage: for _ in range(10000):
....:     T = OrderedTree([])
sage: T.node_number()
RecursionError
```

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#39519
Reported by: Oscfon
Reviewer(s): Frédéric Chapoton, Martin Rubey, Oscfon
vbraun pushed a commit to vbraun/sage that referenced this pull request Mar 3, 2025
sagemathgh-39519: Provide an iterative version on some functions on t…
f88a270 
…rees

    
This pull request provide iterative methods on AbstractTrees which work
both on OrderedTree and BinaryTree.
The problem was that some functions like node_number reach the recursion
limit on huge instances.
```
sage: T = OrderedTree([])
sage: for _ in range(10000):
....:     T = OrderedTree([])
sage: T.node_number()
RecursionError
```

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#39519
Reported by: Oscfon
Reviewer(s): Frédéric Chapoton, Martin Rubey, Oscfon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mantepse mantepse mantepse left review comments

@fchapoton fchapoton fchapoton approved these changes

Assignees

@fchapoton fchapoton

Labels
c: combinatorics s: positive review sd128 tickets of Sage Days 128 Le Teich
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Oscfon @fchapoton @mantepse