-
Notifications
You must be signed in to change notification settings - Fork 862
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
[Finalizing] NAS/Extending Tutorials and Scheduler Fix #31
Conversation
@jwmueller Thanks for reviewing and commenting the code. Sorry that this PR is still work in progress (WIP). I will ping you guys when it is ready to review. |
@@ -0,0 +1,28 @@ | |||
__all__ = ['GridSearcher'] |
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 number of concerns about grid_searcher
.
Will this work for conditional spaces? Doesn't self._configs
contain many invalid options in this case?
I think get_config
is overly simplified here (also doesn't seem like it will necessary work correctly in asynchronous settings). Perhaps it should look like this:
def get_config(self):
if len(self._results) == 0: # no hyperparams have been tried yet, first try default config
config_cs = self.configspace.get_default_configuration()
return config_cs.get_dictionary()
# otherwise return next valid configuration in the grid:
while len(self._configs) > 0:
config_dict = self._configs.pop()
config_cs = convert_dict_to_configpaceConfig(config_dict)
try:
config_cs.is_valid_configuration() # check that it is valid
config_dict = config_cs.get_dictionary()
if (pickle.dumps(config_dict) not in self._results.keys()): # have not yet encountered this config
self._results[pickle.dumps(config_dict)] = 0
return config_dict
except ValueError:
pass
return None # there are no many valid configurations if you reached this 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.
This grid searcher only works for unconditial categorical space. Currently, conditional space is handled in different way in NestedSpace
, we use different key names to handle that. Therefore, there is 'no' conditional space in AutoGluon.
+---------------------------+--------+-----------+ | ||
|
||
|
||
How to reproduce search on EfficientNet? |
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.
more clarity would be good here as well. Something like:
"How to perform the neural architecture search procedure to discover EfficientNet from scratch?"
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 space.* refactor is important for other tasks as discussed in #19
def __repr__(self): | ||
return 'AutoGluonObject' | ||
|
||
class List(NestedSpace): |
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.
Change to categorical?
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.
Categorical
is implemented as we discussed. List
is the original Sequence
, which returns a python list:
ag.space.List(ag.Real(1e-3, 1e-1), ag.Int(0, 5))
will return a list for example:
[0.01, 3]
|
||
def get_hp(self, name): | ||
return CSH.UniformIntegerHyperparameter(name=name, lower=self.lower, upper=self.upper, | ||
default_value=self.default) | ||
|
||
class Bool(Int): |
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 might delete this based on the discussion.
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.
Did we want to remove bool? I thought ag.space.Bool()
is simpler.
@jwmueller @mseeger
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'm fine leaving ag.space.Bool
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 am fine as well, the original discussion is to move it though it is more general
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.
It generally looks good, thanks. Based on our discussion, could you please do three things 1) change ag.* -ag.space.* in all the notebooks and internal usage; 2) delete controller_type == 'atten' since it is only experimental; 3) move efficientnet/mobilenet to contrib
|
for 1) just grep |
addressed 1). |
I vote with Hang that we should keep the |
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.
Generally it looks good and provides important features. I merge this PR to move faster, as discussed, Iet's try to make a single PR focusing on limited functionalities. The original thought is to make the first release feature limited as well. Thanks
script/
folder, due to duplicated information with tutorials.examples/
ag.done()
for shutting downfixes #19
fixes #17
fixes #26
fixes #46
fixes #28