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

MAINT refactor imputation of inactive hyperparameters into function #176

Merged
merged 5 commits into from
Feb 14, 2017

Conversation

mfeurer
Copy link
Contributor

@mfeurer mfeurer commented Feb 13, 2017

Refactor imputation into it's own function @mlindauer

@mfeurer mfeurer requested a review from mlindauer February 13, 2017 16:19
from ConfigSpace.util import get_random_neighbor, get_one_exchange_neighbourhood
from smac.configspace.util import impute_inactive_hyperparameters
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't found such a file. maybe still missing?

@mlindauer
Copy link
Contributor

btw: have you checked that the random forest can handle -1 for categorical parameters?

@mfeurer
Copy link
Contributor Author

mfeurer commented Feb 13, 2017

@sfalkner could you please comment on the question from Marius? It boils down to: does the random forest code from commit 21157707735bc8a11ebf3790a8f5d8de2fcab860 (basically version 0.2 from last April) support -1 as a value for categorical and numerical values?

@sfalkner
Copy link
Contributor

No it doesn't. What would be the purpose of that value anyway?

@mfeurer
Copy link
Contributor Author

mfeurer commented Feb 13, 2017

An outlier value representing a missing value. I'll update this to impute the default, but still be fast.

@sfalkner
Copy link
Contributor

You could use 0 for undefined and use 1...num_categories for the actual values...Don't know if that helps, though. But I guess you wanted the same value for numerical and categorical variables.

@mfeurer
Copy link
Contributor Author

mfeurer commented Feb 13, 2017

Now it's the default again (as it was before). When can decide when the deadlines are over.

@mfeurer mfeurer force-pushed the maint/speedupincativehpimputation branch from c635b6f to 2a336af Compare February 14, 2017 07:36
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 81.778% when pulling 2a336af on maint/speedupincativehpimputation into fa1a843 on development.

1 similar comment
@coveralls
Copy link

coveralls commented Feb 14, 2017

Coverage Status

Coverage increased (+0.1%) to 81.778% when pulling 2a336af on maint/speedupincativehpimputation into fa1a843 on development.

Copy link
Contributor

@mlindauer mlindauer left a comment

Choose a reason for hiding this comment

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

only small notation issues...

@@ -296,12 +296,12 @@ def _build_matrix(self, run_dict, runhistory, instances=None, par_factor=1):
for row, (key, run) in enumerate(run_dict.items()):
# Scaling is automatically done in configSpace
conf = runhistory.ids_config[key.config_id]
conf = impute_inactive_values(conf)
conf_array = impute_inactive_hyperparameters([conf])[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an vector and not an array, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which is the same class in numpy, but I get your point. Will change this.

from smac.configspace import Configuration


def impute_inactive_hyperparameters(configs: List[Configuration]) -> np.ndarray:
Copy link
Contributor

Choose a reason for hiding this comment

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

please add docstring

Copy link
Contributor

Choose a reason for hiding this comment

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

the name of the method is a bit misleading, since it does more than imputation; it also converts it into our internal numpy representation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Docstring is there, how about convert_configurations_to_array?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 81.787% when pulling b638d44 on maint/speedupincativehpimputation into fa1a843 on development.

1 similar comment
@coveralls
Copy link

coveralls commented Feb 14, 2017

Coverage Status

Coverage increased (+0.1%) to 81.787% when pulling b638d44 on maint/speedupincativehpimputation into fa1a843 on development.

@mlindauer
Copy link
Contributor

Thanks!

@coveralls
Copy link

coveralls commented Feb 14, 2017

Coverage Status

Coverage increased (+0.1%) to 81.787% when pulling 423be96 on maint/speedupincativehpimputation into fa1a843 on development.

@mfeurer mfeurer merged commit 1425f78 into development Feb 14, 2017
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.

4 participants