-
Notifications
You must be signed in to change notification settings - Fork 29
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
[MRG] REPS / C-REPS analytical gradient benchmarks #81
[MRG] REPS / C-REPS analytical gradient benchmarks #81
Conversation
@@ -0,0 +1,344 @@ | |||
# Authors: Jan Hendrik Metzen <jhm@informatik.uni-bremen.de> | |||
# Alexander Fabisch <afabisch@informatik.uni-bremen.de> |
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.
You can add yourself here, also in the other files.
from bolero.utils.log import get_logger | ||
|
||
|
||
def solve_dual_contextual_reps(S, R, epsilon, min_eta, approx_grad = True): |
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.
Please use PEP8:
def solve_dual_contextual_reps(S, R, epsilon, min_eta, approx_grad=True):
return d, eta, nu | ||
|
||
|
||
class CREPSOptimizer(ContextualOptimizer): |
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 problem with copying the whole file is that if there are any changes in (c)reps in the future, you have to copy them to the benchmark. You should make a subclass of CREPSOptimizer that overrides set_evaluation_feedback
. The same should be done for the REPSOptimizer.
for i in range(n_trials): | ||
r = eval_loop(Rosenbrock, opt, n_dims, n_iter) | ||
total_time = time.time() - start_time | ||
print name, 'completed in average time of', round(total_time / n_trials, 2), 'seconds' |
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.
We try to be compatible to Python 3 soon. For example:
print("%s: %s %.2f %s" % (name, 'completed in average time of', round(total_time / n_trials, 2), 'seconds'))
total_time = time.time() - start_time | ||
print name, 'completed in average time of', round(total_time / n_trials, 2), 'seconds' | ||
rwds = -np.maximum.accumulate(r) | ||
print name, 'minimum found', rwds[-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.
Please use parentheses for prints.
x = np.zeros(n_dims) | ||
|
||
optimizers = { | ||
"Numerical gradient": REPSOptimizer(x, random_state=0, approx_grad = True), |
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.
PEP8
bolero/optimizer/reps.py
Outdated
@@ -49,15 +49,21 @@ def solve_dual_reps(R, epsilon, min_eta): | |||
|
|||
# Definition of the dual function | |||
def g(eta): # Objective function | |||
return eta * epsilon + eta * logsumexp(R / eta, b=1.0 / len(R)) |
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.
These changes should already be in master, please rebase:
git fetch <remote>
git rebase <remote>/master
# resolve conflicts if there are any
|
||
# Perform the actual optimization of the dual function | ||
if approx_grad: | ||
r = fmin_l_bfgs_b(g, x0, approx_grad=True, bounds=bounds) |
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.
Please don't mix tabs and spaces.
|
||
def show_results(results): | ||
"""Display results.""" | ||
for objective_name, objective_results in results.items(): |
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.
You cannot recognize any difference between both curves. Maybe there is a better way, for example, using a dashed and a solid line.
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 have used a dashed line so that both lines can be seen, but there is no difference between both curves as the solutions found are the same.
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.
Yes, looks better now!
I am not sure if I have rebased properly, I am getting the feeling that I haven't. |
I have definitely messed up with the rebase, please ignore until I fix it. |
The diff looks good but it says I cannot merge. Let me know if you are not able to solve this problem. Maybe this helps? |
@AlexanderFabisch I think the rebase is correct now, let me know if I have missed anything :) |
Mixed tabs and spaces are critical. That should not go to the master. |
Looks good to me apart from that! |
If you zoom in you can really see that the analytical version is performing consistently better. |
Sorry about that, I thought my editor picked those things up. Now flake8 doesn't complain. |
Great, thanks for the contribution! |
My pleasure! |
Benchmarks as suggested in #70