[issue1233] Report unsolvability for bounded search algos if the bound was not used.

[SimonDold]

No description provided.

[roeger]

I see one disadvantage of this, namely that this functionality is easy to miss when one implements a new search algorithm, which can lead to wrong return codes. Alternatively, bound_was_used could be initialized to true in the constructor of SearchAlgorithm, and all algorithms who correctly use it need to update it to false in their constructor. Not beautiful, but safer. What do others think?

[roeger]

Don't you also have to support the flag in the iterated search? I think currently you could wrongly report UNSOLVABLE.

[SimonDold]

I agree with your safety concerns.
I am happy with your suggestion.

Another option could be to have a function like

bool SearchAlgorithm::is_beyond_bound(int x) {
    if (x >= bound) { // greater or equal because the bound is given exclusively
          bound_was_used=true;
          return true;
    }
    return false;
}

And use this function for the checks with the bound.

We can make the field bound private in the base class and remove the getter and setter for it. Only iterated search uses this and there we could deal with a function like

void SearchAlgorithm::shrink_bound(int x) {
    if (x < bound) {
        bound = x;
    }
}

and replace in the iterated search

    if (pass_bound && best_bound < task_specific_search->get_bound()) {
        task_specific_search->set_bound(best_bound);
    }

with

if (pass_bound) {
    task_specific_search->shrink_bound(best_bound);
}

The only remaining case for the field bound to be used is in the log strings.
Like

    log << "Conducting best first search"
        << (reopen_closed_nodes ? " with" : " without")
        << " reopening closed nodes, (real) bound = " << bound << endl;

But for that we could provide a public or protected function like

string SearchAlgorithm::get_bound_string() {
     return to_string(bound);  //fantasy syntax
}

Sure you could read the number out of the string to prune based on the bound without updating that the bound was used :D but that would be noticed in a code review.

[SimonDold]

Yes, you are right. This is already a great example for your safety concerns.

here we would have to update

void IteratedSearch::update_best_status(task_specific_search) {
//...
        if (task_specific_search->get_bound() >= bound) {
            best_status = current_search_status;
        }
//...

This points out a mistake in my earlier comment, where i said get_bound() was only used at one place (i searched in the wrong local branch) for this we could have a function like

int SearchAlgorithm::get_final_bound() {
    if (status == SearchStatus::IN_PROGRESS) {
        ERROR
    }
    return bound;
}

and change the update of the best status to

void IteratedSearch::update_best_status(task_specific_search) {
//...
        if (is_beyond_bound(task_specific_search->get_final_bound())) {
            best_status = current_search_status;
        }
//...

otherwise it should be

void IteratedSearch::update_best_status(task_specific_search) {
//...
       if (ask_specific_search->get_bound() >= bound) {
           bound_was_used = true;
           best_status = current_search_status;
       }
//...

[roeger]

@SimonDold Can you just add the proposed changes to the PR, so that we can see them as a diff? Regarding the proposed method is_beyond_bound, I would use a name that makes the test very clear (whether it is x > bound or x >= bound). Maybe something like is_greater_or_equal_to_bound?

[grucla]

The changes look good to me. I think some renamings / rephrasings would be nice (see comments) but if others prefer the current names I'm fine with merging it as is.

[roeger]

After some consideration, I think that the implementation works correctly for the iterated search (medium confidence). I also like the safer design after the revision (and just am confident that no-one will extract the bound from the string). If everyone else is happy, this can be merged from my side.

[roeger]

Ah, hm, we did not run any experiments for this PR, right? I wonder whether having a call for the bound test for every generated state makes a difference for the runtime.