ENH: Improve Pandas scalars #383
Conversation
bashtage
force-pushed
the
pandas-scalars
branch
from
922e07a to
fbfa4c1
Compare
bashtage
force-pushed
the
pandas-scalars
branch
2 times, most recently
from
44c5c93 to
5d21d0f
Compare
bashtage
force-pushed
the
pandas-scalars
branch
from
5d21d0f to
9c6b963
Compare
There was a problem hiding this comment.
- When checking the operators (
+,-,*,>,<, etc.) for a pandas typePandasType(e.g.Period,Timestamp, etc.) need to checkpandas_type_instance + other_type_instanceandother_type_instance +pandas_type_instance` (Hope that makes sense) - There is some overlap with what is tested in
test_interval.pyandtest_timefuncs.py, so that should get reconciled.
pandas-stubs/_libs/tslibs/period.pyi
Outdated
| @overload # type: ignore[override] | ||
| def __eq__(self, other: _PeriodEqualityComparison) -> bool: ... | ||
| @overload | ||
| def __eq__(self, other: PeriodIndex | DatetimeIndex) -> npt.NDArray[np.bool_]: ... |
There was a problem hiding this comment.
I think the first overload should be object. You can compare a Period to any object. But then you can't make that the first overload. So swap the order, as the first overload would return npt.NDArray[np.bool_] and the second one would be for object returning bool
There was a problem hiding this comment.
Tricky here. For now I've only added types that could actually be either True or False. For always False types, can get array-like[bool] when compared to array-like or bool for scalar types.
|
@bashtage Let me know when I should review again. |
|
@Dr-Irv I think it is ready for review. |
There was a problem hiding this comment.
I think there are still things from my previous review that need to be addressed.
This is getting pretty big, making it a challenge to review, as I'll keep finding things. It also takes a long time to review all of these changes at once. When you're confident that all my comments have been addressed, I then have to go and check that myself, and look at every line again. It's pretty time consuming. When the PR's are smaller, I can turn them around faster by finding small chunks of time here and there. I spent 90 minutes tonight just looking at changes since my last review, and added 41 comments!
I would prefer that this PR be split up into 5 separate PR's. Handle changes for each particular scalar (Interval, Period, Timestamp, Timedelta) as a separate PR for each one, and do one PR for the arrays
| @@ -17,13 +17,17 @@ class IntervalArray(IntervalMixin, ExtensionArray): | |||
| cls, data, closed=..., dtype=..., copy: bool = ..., verify_integrity: bool = ... | |||
| ): ... | |||
| @classmethod | |||
| def from_breaks(cls, breaks, closed: str = ..., copy: bool = ..., dtype=...): ... | |||
| def from_breaks( | |||
| cls, breaks, closed: str = ..., copy: bool = ..., dtype=... | |||
There was a problem hiding this comment.
closed should be IntervalClosedType
| def from_breaks(cls, breaks, closed: str = ..., copy: bool = ..., dtype=...): ... | ||
| def from_breaks( | ||
| cls, breaks, closed: str = ..., copy: bool = ..., dtype=... | ||
| ) -> IntervalArray: ... | ||
| @classmethod | ||
| def from_arrays( | ||
| cls, left, right, closed: str = ..., copy: bool = ..., dtype=... |
There was a problem hiding this comment.
closed should be IntervalClosedType
| @classmethod | ||
| def from_tuples(cls, data, closed: str = ..., copy: bool = ..., dtype=...): ... | ||
| def from_tuples( | ||
| cls, data, closed: str = ..., copy: bool = ..., dtype=... |
There was a problem hiding this comment.
closed should be IntervalClosedType
|
@Dr-Irv I think all of the other PRs except the arrays one are ready for review. |
|
@bashtage should we close this since you split it into multiple PR's? |
assert_type()to assert the type of any return value