-
Notifications
You must be signed in to change notification settings - Fork 140
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
Jialei gh270 connect cuda ei to optimization algorithm #365
Jialei gh270 connect cuda ei to optimization algorithm #365
Conversation
…expected_improvement_gpu_test.cpp
…ct_cudaEI_to_optimization_algorithm Conflicts: moe/optimal_learning/cpp/gpp_core.cpp moe/optimal_learning/cpp/gpp_expected_improvement_gpu.cpp moe/optimal_learning/cpp/gpp_expected_improvement_gpu.hpp moe/optimal_learning/cpp/gpp_expected_improvement_gpu_test.cpp
…github.com:sc932/MOE into jialei_gh270_connect_cudaEI_to_optimization_algorithm
Couple of super broad comments:
|
On Cornell server, it is 149.73s for testing with GPU enabled(including EI/gradEI consistency check and optimization unit test), 92.61s for testing with GPU enabled(without optimization unit test), and 89.85s without GPU. |
~1 minute for the optimization test? Is there any way to make this run faster? Like fewer MC iterations, fewer multistarts or GD steps, etc? lmk when the other changes are up |
…educing num_steps to 100 and increasing tolerance of gradEI at endpoint to 7e-3
} | ||
|
||
/*!\rst | ||
This function is same as ComputeOptimalPointsToSample in gpp_math.cpp, except that it is |
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.
can you put the double backticks around fcn and file names? so:
``ComputeOptimalPointsToSample``
``gpp_math.cpp``
here & elsewhere
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.
fixed
couple more cleanup things |
// special analytic case when we are not using (or not accounting for) multiple, simultaneous experiments | ||
NormalRNG dummy_rng; | ||
EvaluateEIAtPointList(gaussian_process, thread_schedule, initial_guesses, points_being_sampled, num_multistarts, | ||
1, 0, best_so_far, max_int_steps, found_flag, &dummy_rng, function_values, best_next_point); |
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.
instead of "1" and "0", i'd create or use explicit variables with these values so that the reader immediately knows what they mean
also i believe you can pass nullptr in place of &dummy_rng
.
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.
fixed
few more comments + a test for the analytic case as we discussed in person |
one more thing, please update the changelog, thanks! |
move the changelog line and shipit! |
…mization_algorithm Jialei gh270 connect cuda ei to optimization algorithm
********* PEOPLE *************
Primary reviewer: @suntzu86
Reviewers: @sc932
********* DESCRIPTION **************
Branch Name: jialei_gh270_connect_cudaEI_to_optimization_algorithm
Ticket(s)/Issue(s): Closes #270
********* TESTING DONE *************
make test
cpplint.py