search

automl / ParameterImportance

Parameter Importance Analysis Tool
http://www.ml4aad.org/
BSD 3-Clause "New" or "Revised" License
76 stars 19 forks source link

categorical values converted to boolean when reading in trajectory #107

Closed ndangtt closed 4 years ago

ndangtt commented 5 years ago

I'm wondering why categorical values are converted to boolean in the following lines of code? This can be buggy if the domain is not binary, e.g., I have a parameter defined as localsearch {0,1,2,3} [0]. This will cause errors as PIMP convert an incumbent with localsearch=0 to localsearch=False. https://github.com/automl/ParameterImportance/blob/cff1b9017ebd94f7d49fa435f469ae96f5d5e9fb/pimp/importance/importance.py#L270-L271

AndreBiedenkapp commented 5 years ago

Thank you for reporting the issue. When coding that part I did not consider the case you presented above.

I guess we would first need to check if the value is a legal value of the current Categorical hyperparameter and if not convert it too a truthy value :)

@shukon do we have a smarter way of handling that in CAVE?

ndangtt commented 5 years ago

Thanks for your reply! One question I have is if there is any reason why you need this conversion at all? Are you treating binary parameters and categorical parameters differently in your model?

The reason I'm asking is because for example, if I specify a parameter in the .pcs file as param {yes,no} [yes], it actually makes sense to me to see the same domain values in the analysis output rather than True, False.

AndreBiedenkapp commented 5 years ago

No, the values for the model are treated the same, but (at least back when I wrote the code, before json savable ConfigSpace) how they are written to a file is different. So when I actually had a boolean parameter with True, False values, it was saved as string true, false. This lead to the need for parsing the input back to "truthy" values. With the capability to save and load jsonified ConfigSpaces the faulty code could potentially be completely removed.

shukon commented 5 years ago

@AndreBiedenkapp as you already mentioned, I think json is the way to go. I will look into where this problem occurs in our modules.

ndangtt commented 5 years ago

Thank you for your replies! So if I understand it correctly, that would mean the conversion code will be removed? :)

AndreBiedenkapp commented 5 years ago

@ndangtt That is the most likely case :)

ndangtt commented 5 years ago

that's perfect, thank you! :)

shukon commented 5 years ago

Ok, so I'd wait for feedback on https://github.com/automl/SMAC3/pull/555, then adapt appropriate changes in pimp and CAVE to fix backwardscompatibility. Let's hope, json will fix everything ;)

shukon commented 5 years ago

@ndangtt This issue should be fixed in the development-branch now. Please use the development branch until the next release to fix this issue.

ndangtt commented 5 years ago

That's great. Thank you very much for your help! :)