Skip to content

Limiting the branching factor and the depth of branching #674

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

Merged
merged 9 commits into from
Apr 8, 2025

Conversation

msooseth
Copy link
Collaborator

@msooseth msooseth commented Mar 4, 2025

Description

This is a rewrite of the branch width limitation, along with a new branch depth limitation. It should make things a lot cleaner and tidier. Less noise, more sense. Added some tests, too.

Checklist

  • tested locally
  • added automated tests
  • updated the docs
  • updated the changelog

@msooseth msooseth force-pushed the limit-num-branches branch 4 times, most recently from d0f096b to 8167644 Compare March 5, 2025 16:47
@msooseth msooseth changed the title [DRAFT] Limiting the number of branches [DRAFT] Limiting the branching factor and the depth of branching Mar 5, 2025
@msooseth msooseth marked this pull request as ready for review March 11, 2025 12:52
@msooseth msooseth changed the title [DRAFT] Limiting the branching factor and the depth of branching Limiting the branching factor and the depth of branching Mar 11, 2025
@msooseth msooseth force-pushed the limit-num-branches branch 2 times, most recently from 5dcbd8b to b247c89 Compare March 18, 2025 16:10
@msooseth msooseth force-pushed the limit-num-branches branch 2 times, most recently from 40ca150 to 0497634 Compare March 25, 2025 15:08
@msooseth msooseth changed the title Limiting the branching factor and the depth of branching [DRAFT] Limiting the branching factor and the depth of branching Mar 25, 2025
@msooseth msooseth force-pushed the limit-num-branches branch 2 times, most recently from b72d4d9 to 8e252de Compare March 26, 2025 14:53
@msooseth msooseth changed the title [DRAFT] Limiting the branching factor and the depth of branching Limiting the branching factor and the depth of branching Apr 3, 2025
@msooseth
Copy link
Collaborator Author

msooseth commented Apr 3, 2025

@blishko First of all, thanks for the review! I think it helped making this better :) Can you maybe take another peek at this? I tried clarifying the issues you found, hopefully it's in a better state now.

Copy link
Collaborator

@blishko blishko left a comment

Choose a reason for hiding this comment

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

Looks OK to me, though I would like to see some documentation on why you sometime pass the limit to branch, and sometime you explicitly disable it by passing Nothing.
How should one decide when do one or the other?

@msooseth
Copy link
Collaborator Author

msooseth commented Apr 7, 2025

Looks OK to me, though I would like to see some documentation on why you sometime pass the limit to branch, and sometime you explicitly disable it by passing Nothing. How should one decide when do one or the other?

Actually, very good point. Thank you for that! It turns out that I did that only because I didn't have conf.maxExplore in context, so I cheated... It's just wrong. I'll fix it. Thank you, this has been super-helpful, as always!

@msooseth
Copy link
Collaborator Author

msooseth commented Apr 7, 2025

You were spot on. This f4ab216 fixes the issue now. Thank you, you always find some really nice things! Let me know if this fixes the issue :)

Mate

Copy link
Collaborator

@blishko blishko left a comment

Choose a reason for hiding this comment

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

I still lack sufficient knowledge to have a good picture how the exploration proceeds.
But I don't see any obvious issues, so we can try to proceed :)

msooseth and others added 6 commits April 7, 2025 15:07
Fixing up merge

Better layout

Better explanation

Fixing order

Rearrange
Co-authored-by: Martin Blicha <martin.blicha@gmail.com>
Co-authored-by: Martin Blicha <martin.blicha@gmail.com>
Co-authored-by: Martin Blicha <martin.blicha@gmail.com>
Co-authored-by: Martin Blicha <martin.blicha@gmail.com>
@msooseth msooseth force-pushed the limit-num-branches branch 2 times, most recently from cef0aa9 to b2fb6c3 Compare April 7, 2025 17:18
@msooseth msooseth force-pushed the limit-num-branches branch from 74ec66a to 3c1983a Compare April 7, 2025 17:35
Thanks to @blishko for the feedback

Cleanup
@msooseth msooseth force-pushed the limit-num-branches branch from 3c1983a to 8a8387d Compare April 7, 2025 17:38
@msooseth
Copy link
Collaborator Author

msooseth commented Apr 7, 2025

I still lack sufficient knowledge to have a good picture how the exploration proceeds. But I don't see any obvious issues, so we can try to proceed :)

I re-wrote it so hopefully it will be easier to review. It really ought to be easy to review, if not, I likely messed something up... so I decided to rewrite the multi-solution system to be much more clear and straightforward. This should make it so much easier to follow the control flow. The patterns of use of branching should also be much clearer and more consistent now. Honestly, previously, it was a mess, and likely also wrong. We now have a much cleaner --max-width and --max-depth flags, and they do what they say on the lid.

@msooseth msooseth force-pushed the limit-num-branches branch from 8abfe4e to 8a8387d Compare April 7, 2025 18:27
Copy link
Collaborator

@blishko blishko left a comment

Choose a reason for hiding this comment

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

I like the new design! Still have some minor questions, though.

msooseth and others added 2 commits April 8, 2025 11:54
Co-authored-by: Martin Blicha <martin.blicha@ethereum.org>
@msooseth msooseth merged commit ddb822d into main Apr 8, 2025
9 checks passed
@msooseth msooseth deleted the limit-num-branches branch April 8, 2025 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants