-
Notifications
You must be signed in to change notification settings - Fork 19
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 issue 98 by ignoring forbidden clauses #99
base: development
Are you sure you want to change the base?
Conversation
inc_perf, inc_var = self._predict_over_instance_set(impute_inactive_values(self.incumbent)) | ||
|
||
# Create ConfigSpace object without forbidden clauses to use impute_active_values-method | ||
# This can be done because we don't actually use the Configuration-object anywhere |
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 code looks good to me but I don't fully see why this is a valid approach.
Do we now build the EPM using a different configuration space?
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.
Because using the original configuration space yields an error. The impute_inactive_values
-method in the ConfigSpace builds (and returns) a Configuration in which all values that are None are set to their default. If a value is forbidden to be on it's default, an error is raised here.
We don't care about forbidden combinations though (I believe), we just want to estimate over a wide range of Configurations...
Solutions I see are either
- provide an own/ modify ConfigSpace's
impute_inactive_values
-method to allow forbidden vectors
or
- modify the configspace used to create those Configurations, so forbidden combinations can be considered.
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.
Hmmm I see your point. However I don't think this is the correct thing to do.
I think we should catch any configuration that is illegal and leave that part of the config space out of the analysis.
I.e. in case we have two parameters a in [0, 1][0] and b in {0, 1, 2}[1] with a forbidden of b == 2 if a < 0.5.
Now if we want to use LPI around the default of a=0 and b=1, we could never set b = 2 as that is an illegal configuration. In that case we can only compute importances for b in {0, 1} but we could still compute the importance for a over the whole range.
If we just allow forbiddens or replace forbiddens in the config space we would attribute some importance to parts where the only valid importance value would be nan.
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.
Maybe just try and catch ForbiddenValueError
around all calls to impute_inactive_values
?
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.
Should definetly work on LPI side. I'm not sure if that solves it on the fANOVA front.
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.
Apart from semantic's, LPI throws an error for that - presumably because there are no valid neighbors that were sampled at all?
File "/home/shuki/Documents/ParameterImportance/pimp/importance/importance.py", line 503, in evaluate_scenario
self.evaluator.plot_result(os.path.join(save_folder, self.evaluator.name.lower()), show=False)
File "/home/shuki/Documents/ParameterImportance/pimp/evaluator/local_parameter_importance.py", line 323, in plot_result
min_y = min(lower)
TypeError: iteration over a 0-d array
I updated the branch with this solution to have something to work with.
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 I believe in that case LPI didn't encounter any valid neighbors and crashes when trying to plot the results.
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.
Overall this looks like the safe way to handle forbiddens.
If the above error is eliminated I'm happy to accept the PR.
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.
Oh but this is so difficult. With complex forbidden-structures we make the available search space so much smaller. We don't really want to skip the configurations, because they are not per se illegal - I think illegal configurations are filtered before by the _get_one_exchange_neighborhood_by_parameter
-method. We only have valid configurations in the neighborhood, just by forcing all configuration-parameters to take any value, it get's illegal. But with complex forbidden-structures, we kick whole parameters out of analysis... so I think I reframe the problem (and solution):
Problem: to estimate a configuration using the epm, we need to have a value for each dimension (parameter). But for some Configuration, there is no allowed parameter, because the parameter is inactive and forbidden.
Current approach: we just assume default-values for inactive parameters so we can use the epm-interface. This breaks if the parameter is not only specified as inactive (not important) but actually forbidden (doesn't make a semantic difference here!).
Proposed solutions:
- make the epm work with NaNs/empty values/something like this (sounds horribly difficult, but maybe I'm wrong)
or
- ignore the forbidden clauses and proceed as before, just using default values for estimation (epm should ignore them anyway, because they are not important at all)
Fixing #98 as suggested.
Related to automl/CAVE#225