ConfigSpace icon indicating copy to clipboard operation
ConfigSpace copied to clipboard

Correction of `default_value` type in `UniformFloatHyperparameter` and `UniformIntegerHyperparameter`

Open BrunoBelucci opened this issue 2 years ago • 1 comments

This is the solution I came up with after opening issue #336. I have also noted that I could not use Integer() with a default value because the integer default value was being used to initialize the variable self.ufhp, inside UniformIntegerHyperparameter, which defines a UniformFloatHyperparameter with a default value that is an integer if we correctly pass an integer as the default_value argument to Integer. The solution I came up with is to cast to the correct type, but maybe there is a cleaner/clever idea, as I have just started to look into the code and I am not familiar with it.

BrunoBelucci avatar Jul 25 '23 17:07 BrunoBelucci

So, I have looked into it and found out that there were many types that needed to be correctly declared and there are also some incoherences in the declared types across some class inheritances. Besides, many tests seem to be failing even for the main branch, have you recently started to implement those?

Anyway, it should be clear from my commit history which changes I implemented each time. I tried to be somewhat consistent in my choices, but I think that maybe some of the changes need more discussion, for example, if we should accept float parameters (upper, lower, mu, default_value etc) when working with integer parameters. In my opinion, it is a bit strange to define for example an UniformIntegerHyperparameter with a default value that is a float. I see two different solutions to the problem:

  • we consistently accept a float or an integer for every class and we cast the parameters to the correct type afterward
  • we are consistent with what the distribution should represent, for example, for me it makes sense to have float boundaries for a NormalIntegerHyperparameter as long as we return integers, but I don't think that there is much sense in defining float boundaries for a UniformIntegerHyperparameter.

I still have some tests that are falling, but most (if not all of them) are raised because we pass a different type than the class is expecting for some parameters, so I think that we should first discuss if we need to adapt the test or adapt the code.

BrunoBelucci avatar Jul 26 '23 12:07 BrunoBelucci

Hi,

Thank you for all the work you did and apologies things staled out. We dediced to remove Cython entirely in #346 which means typing is no longer critical for compilation. The new PR also ensures type correctness. I will close this as a result but thank you again for the PR :)

eddiebergman avatar Apr 16 '24 19:04 eddiebergman