Skip to content
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

Fix automl handling of positional initializer args #373

Merged
merged 2 commits into from
Jan 15, 2021

Conversation

kbattocchi
Copy link
Collaborator

Fixes #371.

Also fixes a minor drlearner issue.

@kbattocchi kbattocchi requested a review from vsyrgkanis January 15, 2021 00:50

AUTOML_SETTINGS_REG = {
'experiment_timeout_minutes': 1,
'experiment_timeout_minutes': 15,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

15 minutes is the new lowest allowed value for the experiment timeout.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that’s a lot

var = self._get_automated_ml_model(kwarg, key)
new_args += (var,)
if isinstance(arg, EconAutoMLConfig):
arg = self._get_automated_ml_model(arg, f"arg{idx}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the prefix actually used for?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and what if "arg0" is also a keyword argument?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is just used to make it easier for a person to identify the experiment when looking through experiments in AzureML studio, and that the service itself adds a unique suffix so that the whole thing will be unambiguous in any case.

min_samples_leaf=10,
min_impurity_decrease=0.001,
verbose=0, min_weight_fraction_leaf=.01)
est = AutomatedCausalForestDML(model_y=automl_model_reg(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where did the data generation go?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were already performing the data generation in the big try block up top, and we made use of this fact in one test but not the others, which I found confusing.

@kbattocchi kbattocchi merged commit ab70f73 into master Jan 15, 2021
@kbattocchi kbattocchi deleted the kebatt/fix_automl branch January 15, 2021 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in automated_ml integration
2 participants