-
Notifications
You must be signed in to change notification settings - Fork 110
Heuristic Improvements with solution hash, MAB and simplex root solution #216
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
…stic_improvements
|
/ok to test 03ade15 |
aliceb-nv
left a comment
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.
Looking good :) Just a few questions regarding the hashing
Thank you Akif!
| #if !MIP_INSTANTIATE_FLOAT && !MIP_INSTANTIATE_DOUBLE | ||
| static_assert(false, "MIP_INSTANTIATE_FLOAT or MIP_INSTANTIATE_DOUBLE must be defined"); | ||
| #endif |
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.
Those atrocious linking errors :) I wish there was a way to throw an error at preprocessor/link time without having to add explicit checks
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.
Yeah, this is the second time i spend significant amount of time to these issues.
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.
Should we just remove FLOAT? do we anticipate using it for MIP?
| thrust::gather(solution.handle_ptr->get_thrust_policy(), | ||
| solution.problem_ptr->integer_indices.begin(), | ||
| solution.problem_ptr->integer_indices.end(), | ||
| solution.assignment.begin(), | ||
| integer_assignment.begin()); |
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 think we ought to be careful with the implicit cast to size_t performed here. The C++ standard states that any value that cannot be represented in the target cast type yields undefined behavior:
Real floating-integer conversions
A finite value of any real floating type can be implicitly converted to any integer type. Except where covered by boolean conversion above, the rules are:The fractional part is discarded (truncated towards zero).
If the resulting value can be represented by the target type, that value is used
otherwise, the behavior is undefined.
int n = 3.14; // n == 3
int x = 1e10; // undefined behavior for 32-bit int
I think this mean there might be issues in the case of negative and large (>1e20) assignments. It's probably better to compute the hash in a bitwise manner
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.
Good catch, fixed!
| hash_1 ^= hash_2 + 0x9e3779b9 + (hash_1 << 6) + (hash_1 >> 2); | ||
| return hash_1; |
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 think this is a 32-bit hash, with 64-bit values this might not work properly
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.
Thanks fixed!
|
/ok to test 6bcd726 |
|
/ok to test 08d8372 |
aliceb-nv
left a comment
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.
Awesome work Akif!
| solution.problem_ptr->integer_indices.end(), | ||
| solution.assignment.begin(), | ||
| integer_assignment.begin()); | ||
| reinterpret_cast<double*>(integer_assignment.data())); |
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 seriously doubt we will ever use anything but FP64, but in the rare case we instantiate the solver with f_t=FP32, this is going to break/behave badly. We might want to add a static_assert to make sure sizeof(f_t) == sizeof(size_t) in order not to cause issues if users decide to try out FP32
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.
okay let me add the static assert here.
|
/ok to test a92b9d4 |
|
/merge |
Description
This PR adds improvements to heuristics framework with the following: