-
-
Notifications
You must be signed in to change notification settings - Fork 160
GH1641 Pandas 3.0 support #1643
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
base: main
Are you sure you want to change the base?
Conversation
|
I did not remove all the pytest warn bounded, there is already a ton of stuff to look at so thought I would do it in a follow up. @cmp0xff has already made the changes for dropping the dependency on 3.10 so did not do all of those. |
pandas-stubs/core/frame.pyi
Outdated
| col_fill: Hashable = ..., | ||
| drop: _bool = ..., | ||
| inplace: Literal[False] = False, | ||
| inplace: bool = False, |
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.
inplace=True still returns None. Please fix and also add a test.
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.
Reverted and added
| @overload | ||
| def stack( | ||
| self, | ||
| level: IndexLabel = ..., | ||
| *, | ||
| future_stack: Literal[True], | ||
| ) -> Self | Series: ... | ||
| @overload | ||
| def stack( | ||
| self, | ||
| level: IndexLabel = ..., | ||
| dropna: _bool = ..., | ||
| sort: _bool = ..., | ||
| future_stack: Literal[False] = False, | ||
| future_stack: Literal[True] = True, | ||
| ) -> Self | Series: ... |
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.
Both implementations still coexist. I guess we should keep the old implementation.
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.
According to the old warning message, the future_stack=False is a deprecated implementation so I thought we should not let the user use them, unless they willfully ignore the warning. What do you think?
| arr_arrow = cast("ArrowStringArray", pd.array([pd.NA], str)) | ||
| check(assert_type(arr_arrow, ArrowStringArray), ArrowStringArray, float) | ||
| assert pd.isna(assert_type(arr_arrow.dtype.na_value, NAType | float)) | ||
| # pandas-dev/pandas#63567 |
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.
pandas-dev/pandas#54466 will need to be propagated, maybe in a separate PR.
| # pandas-dev/pandas#63567 | |
| # TODO: pandas-dev/pandas#54466 should give BaseStringArray after 3.0 |
| check(pd.array([*data, *data, np.nan], dtype), BaseStringArray, dtype_na) | ||
|
|
||
| if TYPE_CHECKING: | ||
| # TODO: pandas-dev/pandas#54466 should give BaseStringArray after 3.0 |
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.
pandas-dev/pandas#54466 will need to be propagated, maybe in a separate PR.
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.
Yes I plan to do those in a separate one, otherwise it is just a massive bunch of changes and so easy to miss something.
tests/arrays/test_datetime_array.py
Outdated
| check(assert_type(arr.month_name(), np_1darray_object), BaseStringArray) | ||
| check(assert_type(arr.day_name(), np_1darray_object), BaseStringArray) |
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.
The following needs to work
| check(assert_type(arr.month_name(), np_1darray_object), BaseStringArray) | |
| check(assert_type(arr.day_name(), np_1darray_object), BaseStringArray) | |
| check(assert_type(arr.month_name(), BaseStringArray), BaseStringArray) | |
| check(assert_type(arr.day_name(), BaseStringArray), BaseStringArray) |
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.
Fixed
|
|
||
| # interpolate | ||
| if PD_LTE_23: | ||
| check(assert_type(GB_DF.resample("ME").interpolate(), DataFrame), DataFrame) |
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.
Do we need negative tests here?
| if PD_LTE_23: | ||
| with pytest_warns_bounded( | ||
| FutureWarning, | ||
| r"The provided callable <function (sum|mean) .*> is currently using ", |
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.
Probably need to disable the previous implementation and add negative tests in if TYPE_CHECKING_INVALID_USAGE
| # aggregate | ||
| with pytest_warns_bounded( | ||
| FutureWarning, | ||
| r"The provided callable <function (sum|mean) .*> is currently using ", |
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.
Probably need to disable the previous implementation and add negative tests in if TYPE_CHECKING_INVALID_USAGE
| if PD_LTE_23: | ||
| with pytest_warns_bounded( | ||
| FutureWarning, | ||
| r"The provided callable <function (sum|mean) .*> is currently using ", |
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.
Probably need to disable the previous implementation and add negative tests in if TYPE_CHECKING_INVALID_USAGE
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 think something is broken there:
import numpy as np
from pandas import DataFrame
df = DataFrame({"A": ["a"] * 4, "B": range(4)})
df.groupby("A").ewm(1).aggregate(sum) # fails
df.groupby("A").ewm(1).aggregate("sum") # works
df.groupby("A").ewm(1).aggregate(np.sum) # failsRaised issue in pandas-dev/pandas#63855
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.
Seems like they fixed it on main but it will remain broken on 3.0.0, we can decide to ignore the tests until 3.0.1 is released.
| if PD_LTE_23: | ||
| with pytest_warns_bounded( | ||
| FutureWarning, | ||
| r"The provided callable <function (sum|mean) .*> is currently using ", |
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.
Probably need to disable the previous implementation and add negative tests in if TYPE_CHECKING_INVALID_USAGE
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.
Same issue as pandas-dev/pandas#63855
| if TYPE_CHECKING_INVALID_USAGE: | ||
| s.clip(lower=lower, axis=1) # type: ignore[call-overload] # pyright: ignore[reportArgumentType] | ||
| s.clip(lower=lower, axis="column") # type: ignore[call-overload] # pyright: ignore[reportArgumentType] | ||
| s.clip(lower=lower, axis=1) # type: ignore[call-overload] # pyright: ignore[reportCallIssue, reportArgumentType] |
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.
| s.clip(lower=lower, axis=1) # type: ignore[call-overload] # pyright: ignore[reportCallIssue, reportArgumentType] | |
| s.clip(lower=lower, axis=1) # type: ignore[call-overload] # pyright: ignore[reportCallIssue,reportArgumentType] |
| FutureWarning, | ||
| r"Logical ops \(and, or, xor\) between Pandas objects and dtype-less sequences " | ||
| r"\(e.g. list, tuple\) are deprecated", | ||
| lower="2.0.99", |
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.
Maybe negative tests
| FutureWarning, | ||
| r"Logical ops \(and, or, xor\) between Pandas objects and dtype-less sequences " | ||
| r"\(e.g. list, tuple\) are deprecated", | ||
| lower="2.0.99", |
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.
Maybe negative tests
|
I had started a review, then @cmp0xff did a review, then @loicdiridollou pushed new commits, so I will discard that review, and start from the latest commit. Please let me handle this going forward. |
|
I will hold on pushing anything until I get your review, there is a lot of changes so it will be easier like this! |
assert_type()to assert the type of any return value)AGENTS.md.