-
Notifications
You must be signed in to change notification settings - Fork 144
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
Backport some small changes from Scorpion. #207
Backport some small changes from Scorpion. #207
Conversation
@@ -120,7 +121,7 @@ def run_sat(configs, executable, sas_file, plan_manager, final_config, | |||
configs, pos, search_cost_type, heuristic_cost_type, | |||
executable, sas_file, plan_manager, timeout, memory) | |||
if exitcode is None: | |||
return | |||
continue |
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.
If one search has time limit 0 we want to switch to the next config, not abort the whole portfolio.
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.
The rationale behind the original code is that each portfolio should have a non-zero relative time limit, else it shouldn't be part of the portfolio. This means that the time limit hits 0 for all configs at the same time (when the overall time limit is exhausted). So I don't see the purpose of the change. Can you say a scenario where it makes sense?
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.
If config5 has a low relative time limit in the portfolio file, it might get an absolute time limit of 0. But there could be a config6 with a high relative time limit that gets a non-zero absolute time limit.
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.
How? I don't understand. Is there rounding involved that can round down to 0?
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 see now that indeed this is potentially rounded down before we get here. Then the change makes sense, sorry for the noise.
/* IntPacker expects all variables to have at least a domain size of | ||
two. This is not the case for some domain-abstracted tasks. */ | ||
int domain_size = max(2, var.get_domain_size()); | ||
variable_ranges.push_back(domain_size); |
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.
See comment above.
No description provided.