-
Notifications
You must be signed in to change notification settings - Fork 68
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
Enable context for SMAC Optimizer #741
Conversation
mypy fixups
@@ -108,7 +113,7 @@ def __repr__(self) -> str: | |||
) | |||
return f"{self.name}({opt_targets},config={self._config})" | |||
|
|||
def __enter__(self) -> 'Optimizer': | |||
def __enter__(self) -> "Optimizer": |
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.
There's a whole bunch of style-only changes in here that makes reviewing a little difficult.
Can you please revert those, or separate them out to a different PR?
@ agree |
I think we agreed to stall this on until #744 is done to make these changes easier to consume. |
@@ -68,25 +68,30 @@ def __init__(self, | |||
self._seed = int(config.get("seed", 42)) | |||
self._in_context = False | |||
|
|||
experiment_id = self._global_config.get('experiment_id') | |||
experiment_id = self._global_config.get("experiment_id") |
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.
here and everywhere: let's separate cosmetic fixes from functional changes. mixing the two bloats up the diff and makes reviewing PRs much harder. Please keep the essential functionality in this PR and make the diff as small as possible and move all code annotations, style, and docstring improvements to a separate PR. having more PRs is good for your github karma! 😄
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.
Sounds good! #744 should do just the style changes
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, @motus @jsfreischuetz and I already talked about that. #744 is the start of that, but I think will also need some additional work. #746 unearthed some related topics around pyproject.toml
settings vs. setup.cfg
for using black
and how setup
and build
type dependencies are specified (right now we require conda
for certain ones instead of pip
, which as we saw, has some issues).
Am currently working on improvements for all of those and will send a split out series of PRs soon for:
- pyproject.toml settings and build related improvements
- annoyed at this one since there's no "include" feature, so it means duplicating some content across files it looks like
- adding
black
andisort
Makefile
rules,vscode
settings, configs, etc.- that's a part of this one, but I'm going to restrict it to a smaller set so it's easier to see the changes
- requiring them in the build (including all of the reformatting of all files)
- this will be a larger change, so I want make sure it's just reformatting. Can take that up here or just create a new PR
- adding the revision from 3 in the
.gitrevisions
list to ignore it from mostgit blame
types of analysis
Replaced with #751 |
No description provided.