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 issue 98 by ignoring forbidden clauses #99

Draft
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

shukon
Copy link
Collaborator

@shukon shukon commented Feb 12, 2019

Fixing #98 as suggested.
Related to automl/CAVE#225

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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

@shukon shukon Feb 13, 2019

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)

@shukon shukon marked this pull request as draft May 24, 2020 10:54
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.

2 participants