-
Notifications
You must be signed in to change notification settings - Fork 63
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
Conversation
d0f096b
to
8167644
Compare
5dcbd8b
to
b247c89
Compare
40ca150
to
0497634
Compare
b72d4d9
to
8e252de
Compare
@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. |
There was a problem hiding this 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?
Actually, very good point. Thank you for that! It turns out that I did that only because I didn't have |
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 |
There was a problem hiding this 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 :)
Fixing up merge Better layout Better explanation Fixing order Rearrange
Update
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>
cef0aa9
to
b2fb6c3
Compare
74ec66a
to
3c1983a
Compare
Thanks to @blishko for the feedback Cleanup
3c1983a
to
8a8387d
Compare
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. |
8abfe4e
to
8a8387d
Compare
There was a problem hiding this 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.
Co-authored-by: Martin Blicha <martin.blicha@ethereum.org>
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