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

Correction of default_value type in UniformFloatHyperparameter and UniformIntegerHyperparameter #337

Closed
wants to merge 16 commits into from

Conversation

BrunoBelucci
Copy link

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.

…s of type `float` after rounding it. Also ensure that we pass a `float` `default_value` when creating `self.ufhp` inside `UniformIntegerHyperparameter`
Copy link
Contributor

@eddiebergman eddiebergman left a comment

Choose a reason for hiding this comment

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

I'm happy with this change, maybe there is a better solution but this looks exactly like how I would do it! Will run the checks, merge and push to pypi tomorrow hopefully

Copy link
Contributor

@eddiebergman eddiebergman left a comment

Choose a reason for hiding this comment

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

Seems the tests are failing as a result of this. Not entirely sure why, would you be able to have a look at this, I imagine it's probably something to do with the next step self.is_legal().

Whoops, marked it as approved but these changes need to be done :/

# Conflicts:
#	ConfigSpace/hyperparameters/normal_float.pyx
…r (because the parent class only accepts them), maybe need to rethink if we want to accept float
… we cast the int to float at the end it should be good
…f any value is legal, default value must be an integer if we want a normal_INTEGER. Correct test accordingly.
@BrunoBelucci
Copy link
Author

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.

@eddiebergman
Copy link
Contributor

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 :)

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