-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Rerun optimization until valid solution or timeout #53
Conversation
auto const& var = robot.variables[j_idx]; | ||
population_[i].genes[j_idx] = rsl::uniform_real(var.clip_min, var.clip_max); | ||
} | ||
population_[i].genes = robot.get_random_valid_configuration(); |
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.
@tylerjw C++ question: Does switching this implementation from modifying elements of a pre-existing vector to returning a brand new one and overwriting the variable affect performance significantly?
Wondering if it makes more sense for this new function to accept a mutable vector by reference instead.
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.
This is one of the reasons that C++ std algorithms take iterators to existing containers. I don't care either way. I think this is easier to read, and I'd always bias towards easier-to-read code unless you have a benchmark showing that the performance difference matters for your use-case.
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, I think this is fine especially because it only happens when initializing a new population member and not all the time.
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.
Assuming you've tested this in your application, I'm ok with this change.
auto const& var = robot.variables[j_idx]; | ||
population_[i].genes[j_idx] = rsl::uniform_real(var.clip_min, var.clip_max); | ||
} | ||
population_[i].genes = robot.get_random_valid_configuration(); |
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.
This is one of the reasons that C++ std algorithms take iterators to existing containers. I don't care either way. I think this is easier to read, and I'd always bias towards easier-to-read code unless you have a benchmark showing that the performance difference matters for your use-case.
This PR continues rerunning optimization within the timeout budget if the validity callback failed, using a random valid initial seed on each restart.
It also creates a
Robot::get_random_valid_configuration()
member function that is used for this new logic plus in the memetic optimizer. Yay reuse!Closes #49.