-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Use new annotation features in DeltaGenerator #6045
Use new annotation features in DeltaGenerator #6045
Conversation
) -> Optional[DG]: | ||
data: Data = None, | ||
**kwargs: DataFrame | ||
| npt.NDArray[Any] |
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'm not 100% sure, but I think there was some kind of problem with older numpy versions if NDArray
is used that way. The reason is that NDArray
was only added a few versions ago. Maybe you can check if everything works fine if you select the oldest numpy version we support.
) -> Optional[DG]: | ||
data: Data = None, | ||
**kwargs: DataFrame | ||
| npt.NDArray[Any] |
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.
See the comment above
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.
LGTM 👍 The only aspect I would suggest to check here if there are any issues with older numpy versions with the usage of the NDArray
type (this issue might actually be solved by the usage of the new annotation features).
Appears to work fine (no errors when running tests) on numpy 1.18, which is as far back as I can readily install in my dev env. So I think it's fine. The change is about making use of lazy annotations, so nothing in them should matter for runtime. |
📚 Context
Continuing the gradual adoption of
from __future__ import annotations
What kind of change does this PR introduce?