-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Generic PrimitiveJob and BaseSampler/BaseEstimator #9920
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 4685190880
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's useful to do this, but I don't think you need to override the derived methods to add the type hints. Deriving a class from Generic
means you can define the specialisation during the inheritance, rather than at method resolution. For example, given a file generic.py
:
import typing
T = typing.TypeVar("T")
class Base(typing.Generic[T]):
def f(self) -> T:
raise NotImplementedError
class Int(Base[int]):
pass
class Str(Base[str]):
pass
reveal_type(Base().f())
reveal_type(Int().f())
reveal_type(Str().f())
the result of running mypy on it is:
generic.py:15: note: Revealed type is "<nothing>"
generic.py:16: note: Revealed type is "builtins.int"
generic.py:17: note: Revealed type is "builtins.str"
Success: no issues found in 1 source file
@jakelishman Thanks. That's true, but to do so would require making |
Ah sorry, I'd misread the code a bit. Yeah, in the way the primitives typing is currently done, the way you've done things is the best. Making |
@jakelishman Thank you for your advice. I updated the code. Is this correct? |
I believe #9480 will fix the lint error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't comment on whether the type bounds you've chosen are precisely what the interface is meant to be (I assume that you're correct), but the implementation of them looks fine to me.
I can re-approve once the conflicts are fixed. Looks like it's just super minor stuff. |
@jakelishman Thank you very much! For the bound, this PR does not should neither strengthen nor weaken the restrictions of the type. |
* Generic PrimitiveJob * Generic BaseSampler/BaseEstimator * remove type hint in _run
* Generic PrimitiveJob * Generic BaseSampler/BaseEstimator * remove type hint in _run
Summary
This PR improves type hinting with
Generic
.The type of result returned by
Primitives.run
is made explicit, so it is enabling code completion and improving the user experience.Details and comments