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.

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

Provide an iterative version on some functions on trees #39519

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

Conversation

Oscfon
Copy link

@Oscfon Oscfon commented Feb 13, 2025

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

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.

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

last fix for the linter
Copy link

github-actions bot commented Feb 18, 2025

Documentation preview for this PR (built with commit f35c7d6; 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: combinatorics s: needs review sd128 tickets of Sage Days 128 Le Teich
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants