-
Notifications
You must be signed in to change notification settings - Fork 8
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
Reuse hyperparameters #41
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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 a few non-blocking suggestions below.
Let's make sure we add docstrings to all public functions/classes within the documentation improvement efforts.
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 name of this module needs to be changed, otherwise, pytest may pick it up (although won't execute any tests).
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 we just add addopts = --ignore=somedir
to pytest.ini
?
def constrained_himmelblau(x1, x2): | ||
if x1**2 + x2**2 > 50: | ||
return np.nan | ||
return himmelblau(x1, x2) | ||
|
||
|
||
def skewed_himmelblau(x1, x2): | ||
_x1 = 2 * x1 + x2 | ||
_x2 = 0.5 * (x1 - 2 * x2) | ||
|
||
return constrained_himmelblau(_x1, _x2) | ||
|
||
|
||
def bukin(x1, x2): | ||
return 100 * np.sqrt(np.abs(x2 - 1e-2 * x1**2)) + 0.01 * np.abs(x1) |
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.
All these functions need a docstring with a reference to Wikipedia or other resources.
RE(agent.learn("ei", n_iter=2)) | ||
RE(agent.learn("pi", n_iter=2)) |
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.
All these options need to be listed in the general documentation (in Intro section, not in tutorials (=notebooks)).
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'll make a PR with improved documentation outside the tutorials, for this and the above
def dummy_dof(name): | ||
return Signal(name=name, value=0.0) | ||
|
||
|
||
def dummy_dofs(n=2): | ||
return [Signal(name=f"x{i+1}", value=0) for i in range(n)] | ||
return [dummy_dof(name=f"x{i+1}") for i in range(n)] | ||
|
||
|
||
def get_dummy_device(name="dofs", n=2): |
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.
Docstrings should be added.
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 overhauled in the next PR
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.
Looks good, let's incorporate it in other PRs.
Reuse hyperparameters
This introduces functionality to save/specify a priori hyperparameters on initialization, which speeds up optimization.