-
-
Notifications
You must be signed in to change notification settings - Fork 147
Add numpy array shapes for return types #1325
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
Conversation
The mypy test failures in CI (first couple commits) are weird. They appear on Python 3.10 only and only in CI. I couldn't reproduce them locally even with the same versions of python, mypy, pandas and numpy as in CI. I skip these test only for mypy and only on Python 3.10. |
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.
Thanks for this PR. Very interesting stuff.
I figured out your problem. If you look at my changes in 1db78e8 (and maybe 1db78e8, although I think the first commit I listed has everything), the changes I made will make it work for mypy with python 3.10, and allow you to use ShapeT
in Timestamp.__eq__()
and Timestamp.__ne__()
I was able to replicate the python 3.10 problem locally, making it easier to debug.
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.
forgot one comment
@Dr-Irv this is ready for another round of review |
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.
Thanks for the nice work on this and reorganizing the tests in test_scalars.py
.
There is one set of tests to add which are testing that when we get to_numpy()
on an Index
or Series
, we know that we have a 1D array, so we should test that comparing that 1D array preserves the 1D array of the boolean result.
@Dr-Irv this is implemented. To have this working, I had to add the numpy dtype to the In the future I may revisit making |
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.
Very close. One small thing in the tests that I think should be changed, but I could be convinced otherwise.
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.
thanks @hamdanal . Nice contribution.
thank you! |
assert_type()
to assert the type of any return valueI made these changes:
check
function in the tests to validate the shape and dtype of numpy arrays