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

quniform still gives float values instead of int #508

Closed
f90 opened this issue May 29, 2019 · 9 comments
Closed

quniform still gives float values instead of int #508

f90 opened this issue May 29, 2019 · 9 comments
Labels

Comments

@f90
Copy link

f90 commented May 29, 2019

quniform is still giving me a float value instead of an int, just like described in issue #253.
It was supposedly fixed, but for me the problem persists. See the "epochs" variable below in this minimum working example.

from hyperopt import hp
import hyperopt

space = [hp.loguniform('lr', -12, -1),
         hp.quniform('epochs', 10, 100, 1),
         hp.choice('clip_decision', [-1, hp.uniform('clip', 0.01, 1.0)])]
print(hyperopt.pyll.stochastic.sample(space))

Result:

(0.03721354540878709, 70.0, 0.5326563351349751)

I am using Python 3.6 with hyperopt package 0.1.2

@bjkomer
Copy link
Member

bjkomer commented May 29, 2019

I dug into this a little. Seems like the fix in #444 is being negated by this line in pyll_utils which converts it back into a float again. The test missed this because it skips that pathway and uses quniform_gen directly. I'm not sure of the reasoning behind needing a float conversion there or the consequences of removing it.

To get an int in your code with the current version of hyperopt that you have, you can explicitly cast it to an int like this:

from hyperopt.pyll.base import scope
from hyperopt import hp
import hyperopt

space = [hp.loguniform('lr', -12, -1),
         scope.int(hp.quniform('epochs', 10, 100, 1)),
         hp.choice('clip_decision', [-1, hp.uniform('clip', 0.01, 1.0)])]
print(hyperopt.pyll.stochastic.sample(space))

which gives this result:

(0.007870765528775695, 75, 0.41810290675346484)

@f90
Copy link
Author

f90 commented May 30, 2019

Thanks, this is a good workaround for now. You also did some great error analysis. Hopefully someone else can pitch in and provide a pull request/fix for this to prevent more people from running into this issue. Also maybe other discrete distributions might be affected similarly, so it would be a good idea to check the other distributions along the way.

@stephenleo
Copy link

Thanks for the workaround

@jona-sassenhagen
Copy link

I dug into this a little. Seems like the fix in #444 is being negated by this line in pyll_utils which converts it back into a float again. The test missed this because it skips that pathway and uses quniform_gen directly. I'm not sure of the reasoning behind needing a float conversion there or the consequences of removing it.

The fix would be simply swapping out the scope.float in pyll_utils for scope.int correct? I wrote a tiny patch + unit test. Should I open a PR?

@bjkomer
Copy link
Member

bjkomer commented Jul 3, 2019

@jona-sassenhagen swapping it for scope.int will just give the opposite problem of it giving an int instead of a float all of the time. In general the output of quniform should be a float.

It may work to just remove the scope.float entirely, but I'm not sure if that has downstream consequences. There are lots of little pieces like this function that don't seem to do much but are used for optimization. These explicit type conversions with scope might be used in a similar way, I just haven't looked into it and don't know.

Likely the best thing would to have it choose to use scope.int or scope.float depending on the arguments to quniform

@jona-sassenhagen
Copy link

That makes sense - I can see about that, too. Am I understanding you correctly that you envision a new parameter for quniform? Something like int=False or dtype=(int, float)?

@bjkomer
Copy link
Member

bjkomer commented Jul 3, 2019

I was thinking it could infer it from the type of q given so there wouldn't need to be any extra arguments.

Just noticed there is already an integer version of quniform called uniformint that could be used instead for most cases. The code is here. It uses a fixed step size of 1 though

@PhilipMay
Copy link
Contributor

I noticed that uniformint is not mentioned on the Wiki: https://github.com/hyperopt/hyperopt/wiki/FMin#21-parameter-expressions

Copy link

This issue has been marked as stale because it has been open 120 days with no activity. Remove the stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Apr 30, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants