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

Add option to stop search once a solution is found #13

Merged

Conversation

AustinT
Copy link
Collaborator

@AustinT AustinT commented Jul 11, 2023

Rationale: Even though our main use case for syntheseus so far has been to run search for a fixed amount of time and analyze all routes in the resulting search graph, there are some scenarios where we only care about whether or not a route is found (e.g. benchmarking). This is pretty simple to check so I thought it is worth supporting.

Implementation: previously all algorithms called time_limit_reached each iteration to decide whether to stop early. I just augmented this function to also check for stopping once a solution is found. This involved:

  1. Adding a stop_on_first_solution kwarg to the base algorithm class (defaults to False, which is the most sensible default in my opinion)
  2. Adding logic to time_limit_reached which checks self.stop_on_first_solution and graph.root_node.has_solution. This required adding a graph argument to the function (previously it took no arguments), causing changes in all algorithm files.
  3. Because of this added functionality, time_limit_reached no longer seemed like a descriptive name for the function, so I changed it to should_stop_search
  4. I added a test to syntheseus/tests/search/algorithms/test_base.py which tests the effectiveness of this kwarg for all algorithms (because algorithm-specific tests all subclass the base test)

Rejected alternative: although it would be appealing to generalize this to "stop after n solutions" or "stop after n diverse solutions" this is not simple or quick to check so I don't think we should support it out-of-the-box. If users want this then they could implement it themselves in a subclass.

@AustinT
Copy link
Collaborator Author

AustinT commented Jul 11, 2023

@AustinT please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@AustinT AustinT requested a review from kmaziarz July 11, 2023 12:03
@kmaziarz
Copy link
Contributor

there are some scenarios where we only care about whether or not a route is found (e.g. benchmarking)

Can you elaborate on this usecase?

Copy link
Contributor

@kmaziarz kmaziarz left a comment

Choose a reason for hiding this comment

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

LGTM!

syntheseus/search/algorithms/base.py Outdated Show resolved Hide resolved
syntheseus/tests/search/algorithms/test_base.py Outdated Show resolved Hide resolved
@AustinT
Copy link
Collaborator Author

AustinT commented Jul 12, 2023

there are some scenarios where we only care about whether or not a route is found (e.g. benchmarking)

Can you elaborate on this usecase?

For example, if you wanted to reproduce the experiment in the retro* / PDVN paper, or make a box plot of the time at which the first solution is found, this only requires finding on solution (that is all they report). The students I am working with are trying to evaluate value functions in terms of how quickly they guide a search algorithm to a solution. That is another use case. Does that make sense?

Co-authored-by: Krzysztof Maziarz <krzysztof.maziarz@microsoft.com>
@AustinT AustinT merged commit 6465917 into microsoft:main Jul 13, 2023
@AustinT AustinT deleted the austin/2023-july11-feat-stop-on-first-solution branch July 13, 2023 13:53
kmaziarz added a commit that referenced this pull request Sep 9, 2024
In #13, a `stop_on_first_solution` flag was added to the base search
algorithm class, but so far it was not available when using the search
CLI. As this option can be quite useful (for e.g. running a quick check
to assess the difficulty of a given set of targets), this PR adds it as
a CLI option as well.
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