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

Issue1130 #203

Merged
merged 4 commits into from
Jul 10, 2024
Merged

Issue1130 #203

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3677d28
Use bounds of component algorithms in iterated search.
ClemensBuechner Jan 8, 2024
32d8798
Prettify unrelated code.
ClemensBuechner Jan 16, 2024
cf95dc5
Add document note.
ClemensBuechner Jan 22, 2024
d65e3db
Update documentation.
ClemensBuechner Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/search/search_algorithms/iterated_search.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ shared_ptr<SearchAlgorithm> IteratedSearch::create_current_phase() {
this overrides continue_on_fail.
*/
if (repeat_last_phase && last_phase_found_solution) {
return get_search_algorithm(
algorithm_configs.size() -
1);
return get_search_algorithm(algorithm_configs.size() - 1);
} else {
return nullptr;
}
Expand All @@ -63,7 +61,7 @@ SearchStatus IteratedSearch::step() {
if (!current_search) {
return found_solution() ? SOLVED : FAILED;
}
if (pass_bound) {
if (pass_bound && best_bound < current_search->get_bound()) {
current_search->set_bound(best_bound);
}
++phase;
Expand Down Expand Up @@ -185,6 +183,18 @@ class IteratedSearchFeature
"(using heuristic predefinition) between iterations, "
"the path data (that is, landmark status for each visited state) "
"will be saved between iterations.");
document_note(
"Semantics of Search Bounds",
"The definition of our interfaces for search algorithms allow to "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"The definition of our interfaces for search algorithms allow to "
"Our interfaces for search algorithms allow to "

"set bounds for the iterated search and all its component search "
Copy link
Contributor

Choose a reason for hiding this comment

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

This construction with "allow to" is apparently a typical English language mistake that native speakers of German do. Chris Beck pointed this out in the context of Florian's thesis.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'm also susceptible to the mistake, so I'm not 100% sure about the following. But I think the error is that "allow" requires saying who is allowed something, i.e., you need a direct object like "allow the users to ...". The better solution is almost always to use a different phrasing.

"algorithms individually. We do not see a reasonable use case "
"where setting a bound for a component search algorithm is "
"advisable. If this is done anyways, the bound of said search "
Copy link
Contributor

Choose a reason for hiding this comment

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

"anyways" is not really a world (in formal writing); it's always "anyway".

But also I think this discussion is much longer than I think it needs to be. I would not discuss what is or isn't reasonable and discuss interactions with "pass_bound" at length. I would limit this for a discussion of what the behaviour is, and I think it's enough to make this part of the discussion of "pass_bound" rather than a separate note.

I struggle a bit to formulate this because I think the text for "pass_bound" is already bad. What is the "bound from previous search"? We're not using a bound from a previous search, we use the best solution cost so far as a bound.

I would not add a new note and instead rewrite the documentation of "pass_bound", for example along those lines: "use the cost of the best solution found as a bound for component algorithms, unless these already have a lower bound set".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's a good idea to integrate the document note into the documentation of the pass_bound option. I also like the suggested phrasing, except that the passed bound does not have to be based on the best solution so far, it could also just be the bound set for the iterated search itself. So how about this instead:

"use the cost of the best solution found (or bound of iterated search if none found so far) as a bound for component algorithms, unless these already have a lower bound set".

Copy link
Member

Choose a reason for hiding this comment

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

How about this as a replacement for the current pass_bound option and instead of the extra document note?

        add_option<bool>(
            "pass_bound",
            "use the real cost of the plan (regardless of the cost_type "
            "parameter) found in one iteration as a bound for the next "
            "iteration. If that search for that iteration already has a "
            "bound, the lower of the two bounds is used.",
            "true");

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, my comment overlapped with yours and wasn't meant as a response. Do we want to keep the part about the cost type and real cost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the "real cost (regardless of cost_type)" is relevant information, so combining the suggestions by Florian and me, I ended up with the following draft:

"use the bound of iterated search as a bound for its component search algorithms, unless these already have a lower bound set. The iterated search bound is tightened whenever a component finds a cheaper plan (using real costs regardless of the cost_type parameter)."

What are your opinions?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good to me

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer Clemens's phrasing over Florian's suggestion because it is more precise without being convoluted. ("found in one iteration as a bound for the next iteration" is not quite right because it is used for all following iterations, not just the next one).

The discussion of cost_type invites the question what cost_type for iterated does then, and the answer is: it does nothing and shouldn't be there in the first place.

It would be good to fix this sooner rather than later. Its existence is a consequence of the currently wrong inheritance hierarchy for search algorithms, where too much stuff is present in the base class.

Personally, I have a mild preference for not including a discussion of cost_type in the bound documentation because I think it doesn't resolve the confusion that people might have about cost_type, and it's not unlikely that once we fix the hierarchy, we will forget to update the documentation of this argument. But if you prefer to include it, I won't object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Florian and I support removing the cost_type discussion now. I have removed the comment in parentheses.
Fixing the inheritance hierarchy should be done in a separate issue, though.

"iteration is set to the minimum of that bound and the overall "
"bound on the iterated search. This can be particularly relevant "
"when the option `pass_bound` is set to true in which case the "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"when the option `pass_bound` is set to true in which case the "
"when the option `pass_bound` is set to true, in which case the "

"bound of the iterated search is updated whenever a better "
"solution is found by one of its components.");
}

virtual shared_ptr<IteratedSearch> create_component(const plugins::Options &options, const utils::Context &context) const override {
Expand Down
Loading