bayesian-optimization / BayesianOptimization

A Python implementation of global optimization with gaussian processes.
https://bayesian-optimization.github.io/BayesianOptimization/index.html
MIT License
7.95k stars 1.55k forks source link

support PEP 484 #493

Closed phi-friday closed 2 months ago

phi-friday commented 4 months ago

Is your feature request related to a problem? Please describe.

No type hint is written, resulting in a static analysis that differs from annotations. ex: xi accepts float, but is inferred to be int because of the default.

스크린샷 2024-07-09 오전 9 04 26

This also causes some issues with static analysis and using the IDE.

Describe the solution you'd like Add the correct type hint.

References or alternative approaches https://peps.python.org/pep-0484/

Additional context If you decide to support type hint, I'll contribute. However, since a lot of changes are planned in #447 , i'll start with modules that are not related to #447 .

Are you able and willing to implement this feature yourself and open a pull request?

phi-friday commented 4 months ago

I have one question while writing #494. TargetSpace.register() requires float as constraint_value, and if constraint_value is restricted to float. then the return type of ConstraintModel will be fixed to float, and the value of constraint in TargetSpace.max() will also be fixed to float.

However, when I checked logger.py, it seems to happen in the case of np.ndarray.

Is the comment in TargetSpace.register() wrong?

till-m commented 4 months ago

In case of multiple constraints this can indeed be an np.ndarray. See e.g. this test.

till-m commented 4 months ago

That is to say -- yes, I wrote documentation that's wrong 😬

phi-friday commented 4 months ago

In case of multiple constraints this can indeed be an np.ndarray. See e.g. this test.

Thanks, that makes it a little clearer where to fix it.

By the way, can I continue with this #494 ?

till-m commented 4 months ago

By the way, can I continue with this https://github.com/bayesian-optimization/BayesianOptimization/pull/494 ?

I'm personally in favour of adding type hints everywhere. It hasn't been a priority since I consider it more of a QoL update, additionally I don't have a lot of experience with type hints and handling more complex situations. I quickly had a look at your PR and I'm a bit surprised that some things are not immediately clear to me in their function (TypeVar, Generic, ParamSpec, etc.; i.e. most of the non-primitive type imports from typing), though I am also not a software engineer and I'm not familiar with more advanced type hinting. I don't mind learning something, but it's important to me that this package's code remains accessible for casual users. One thing that I like about this package is that it is very easy for non-experienced programmers to contribute if they know some python, I am worried that this will increase the threshold for such contributions.

There may simply not be a better way of handling things, I'm unfortunately not qualified to answer that. I would suggest you continue for now and we have another look at review time. What do you think?

phi-friday commented 4 months ago

By the way, can I continue with this #494 ?

I'm personally in favour of adding type hints everywhere. It hasn't been a priority since I consider it more of a QoL update, additionally I don't have a lot of experience with type hints and handling more complex situations. I quickly had a look at your PR and I'm a bit surprised that some things are not immediately clear to me in their function (TypeVar, Generic, ParamSpec, etc.; i.e. most of the non-primitive type imports from typing), though I am also not a software engineer and I'm not familiar with more advanced type hinting. I don't mind learning something, but it's important to me that this package's code remains accessible for casual users. One thing that I like about this package is that it is very easy for non-experienced programmers to contribute if they know some python, I am worried that this will increase the threshold for such contributions.

There may simply not be a better way of handling things, I'm unfortunately not qualified to answer that. I would suggest you continue for now and we have another look at review time. What do you think?

If you want to keep runtime code as it is, you can also support type hint with a stub file(*.pyi). In fact, pandas supports it using stub file.

However, you will need to synchronize the stub file with the runtime code, which can make supporting type hints a bit complicated.

I'm used to typehint, so it doesn't matter to me, but I agree with what you said. I think we'll have to wait until #447 is done before we can finish this anyway, so let's keep it as is for now, and let me know when you've decided and we can move forward.

till-m commented 2 months ago

Hey @phi-friday, do we consider this issue fixed already, fixed after merging #520 or is there something else left to be done?

phi-friday commented 2 months ago

Hey @phi-friday, do we consider this issue fixed already, fixed after merging #520 or is there something else left to be done?

In my opinion, it's practically already done, but I think adding py.typed would make it clearer. This issue will be automatically closed with the merge of #520. You don't need to do anything else. Of course, you can just close it.

till-m commented 2 months ago

Thanks for your contributions! Much appreciated!

phi-friday commented 2 months ago

@till-m Thank you for actively sharing your thoughts with me. It helped me to see this issue through to completion.