-
Notifications
You must be signed in to change notification settings - Fork 299
[FIX] SWA and SE with non cyclic schedulers #395
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
Conversation
Codecov Report
@@ Coverage Diff @@
## reg_cocktails #395 +/- ##
=================================================
+ Coverage 82.89% 83.46% +0.56%
=================================================
Files 172 172
Lines 10336 10369 +33
Branches 1815 1827 +12
=================================================
+ Hits 8568 8654 +86
+ Misses 1244 1184 -60
- Partials 524 531 +7
Continue to review full report at Codecov.
|
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.
Hi Thanks for the PR.
I checked except base_trainer.
exclude=exclude if bool(exclude) else None, | ||
include=include if bool(include) else None) |
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.
Do we need to use if statements here?
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 because our get_available_components
logic checks if include
or exclude
are None
or not. I think though we dont need to use get_available_components
as I only need the string names. I'll fix this.
# Disable CyclicLR until todo is completed. | ||
if 'lr_scheduler' in self.named_steps.keys() and 'trainer' in self.named_steps.keys(): | ||
trainers = cs.get_hyperparameter('trainer:__choice__').choices | ||
for trainer in trainers: | ||
available_schedulers = self.named_steps['lr_scheduler'].get_available_components( | ||
dataset_properties=dataset_properties, | ||
exclude=exclude if bool(exclude) else None, | ||
include=include if bool(include) else None) | ||
# TODO: update cyclic lr to use n_restarts and adjust according to batch size | ||
cyclic_lr_name = 'CyclicLR' | ||
if cyclic_lr_name in available_schedulers: | ||
# disable snapshot ensembles and stochastic weight averaging | ||
cs.add_forbidden_clause(ForbiddenAndConjunction( | ||
ForbiddenEqualsClause(cs.get_hyperparameter( | ||
f'trainer:{trainer}:use_snapshot_ensemble'), True), | ||
ForbiddenEqualsClause(cs.get_hyperparameter('lr_scheduler:__choice__'), cyclic_lr_name) | ||
)) | ||
cs.add_forbidden_clause(ForbiddenAndConjunction( | ||
ForbiddenEqualsClause(cs.get_hyperparameter( | ||
f'trainer:{trainer}:use_stochastic_weight_averaging'), True), | ||
ForbiddenEqualsClause(cs.get_hyperparameter('lr_scheduler:__choice__'), cyclic_lr_name) | ||
)) |
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 make a function for it because we do the same thing for both tabular_{classification, regression}?
And this method is long.
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.
sure
autoPyTorch/pipeline/components/setup/network_embedding/__init__.py
Outdated
Show resolved
Hide resolved
updates = self._get_search_space_updates() | ||
if '__choice__' in updates.keys(): | ||
choice_hyperparameter = updates['__choice__'] | ||
if not set(choice_hyperparameter.value_range).issubset(available_embedding): | ||
raise ValueError("Expected given update for {} to have " | ||
"choices in {} got {}".format(self.__class__.__name__, | ||
available_embedding, | ||
choice_hyperparameter.value_range)) | ||
if len(categorical_columns) == 0: | ||
assert len(choice_hyperparameter.value_range) == 1 | ||
if 'NoEmbedding' not in choice_hyperparameter.value_range: | ||
raise ValueError("Provided {} in choices, however, the dataset " | ||
"is incompatible with it".format(choice_hyperparameter.value_range)) | ||
embedding = CSH.CategoricalHyperparameter('__choice__', | ||
choice_hyperparameter.value_range, | ||
default_value=choice_hyperparameter.default_value) | ||
else: | ||
|
||
if len(categorical_columns) == 0: | ||
default = 'NoEmbedding' | ||
if include is not None and default not in include: | ||
raise ValueError("Provided {} in include, however, the dataset " | ||
"is incompatible with it".format(include)) | ||
embedding = CSH.CategoricalHyperparameter('__choice__', | ||
['NoEmbedding'], | ||
default_value=default) | ||
else: | ||
embedding = CSH.CategoricalHyperparameter('__choice__', | ||
list(available_embedding.keys()), | ||
default_value=default) |
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 create a method for this?
def _get_embedding_config(self, updates, avail_embeddings, default: str = "NoEmbedding"):
if '__choice__' in updates.keys():
choices, default = updates['__choice__'].value_range, updates['__choice__'].default_value
if not set(choices).issubset(avail_embeddings):
raise ValueError(
f"The choices for {self.__class__.__name__} must be in {avail_embeddings}, but got {choices}"
)
if len(categorical_columns) == 0:
assert len(choices) == 1
if 'NoEmbedding' not in choices:
raise ValueError(f"The choices must include `NoEmbedding`, but got {choices}")
embedding = CSH.CategoricalHyperparameter('__choice__', choices, default_value=default)
elif len(categorical_columns) == 0:
default = 'NoEmbedding'
if include is not None and default not in include:
raise ValueError(f"default `{default}` must be in `include`: {include}")
embedding = CSH.CategoricalHyperparameter('__choice__', ['NoEmbedding'], default_value=default)
else:
choices = list(available_embedding.keys())
embedding = CSH.CategoricalHyperparameter('__choice__', choices, default_value=default)
return embedding
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.
let me do that after we are done with the experiments. There is an issue raised for this. #377 .
autoPyTorch/pipeline/components/training/trainer/base_trainer.py
Outdated
Show resolved
Hide resolved
autoPyTorch/pipeline/components/training/trainer/base_trainer.py
Outdated
Show resolved
Hide resolved
model_copy = deepcopy(self.swa_model) if self.use_stochastic_weight_averaging \ | ||
else deepcopy(self.model) |
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.
Is it ok to add the condition is_last_epoch
in the new implementation?
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.
what do you mean?
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.
He caught a bug, when there was a wrong implementation initially for se and swa when both activated and it was fixed, it was not fixed for the case of a non-cyclic scheduler. Hence now that you fixed it, it is different from the previous implementation since you had no if last epoch check to add the swa model only in the end, but you add 3 copies of it throughout.
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.
Basically, now it is ok and correct.
Co-authored-by: nabenabe0928 <47781922+nabenabe0928@users.noreply.github.com>
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 had one small question and except that everything looks good, especially refactoring to remove the code duplication.
)) | ||
break | ||
except ValueError: | ||
# change the default and try again |
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.
Why would we get an error here and why do we need to change the default?
It is not the case that we might have a default value that is not in the list of choices right?
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 will get a value error in case LearnedEntityEmbedding
was the default value for the __choice__
hyperparameter
* Enable learned embeddings, fix bug with non cyclic schedulers * add forbidden condition cyclic lr * refactor base_pipeline forbidden conditions * Apply suggestions from code review Co-authored-by: nabenabe0928 <47781922+nabenabe0928@users.noreply.github.com> Co-authored-by: nabenabe0928 <47781922+nabenabe0928@users.noreply.github.com>
* Enable learned embeddings, fix bug with non cyclic schedulers * add forbidden condition cyclic lr * refactor base_pipeline forbidden conditions * Apply suggestions from code review Co-authored-by: nabenabe0928 <47781922+nabenabe0928@users.noreply.github.com> Co-authored-by: nabenabe0928 <47781922+nabenabe0928@users.noreply.github.com>
This PR fixes a bug in swa and se when they are used with noncyclic schedulers. Also enables learned entity embeddings for reg_cocktails.