-
-
Notifications
You must be signed in to change notification settings - Fork 152
feat: reduce np.ndarray #1462
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?
feat: reduce np.ndarray #1462
Conversation
| ) -> tuple[np.ndarray, np.ndarray]: ... | ||
| ) -> tuple[npt.NDArray[Any], 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.
is this a chance to make this more precise, like
tuple[np_1d_array[np.intp], np_1d_array[np.intp]]?
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 have no insight into this class / this class method, nor is it documented in any detail (personally I don't even know what "window bounds" means). It would be great if you suggest a few tests, or can we leave it in another 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.
sure happy to leave it til later!
|
|
||
| class CategoricalIndex(ExtensionIndex[S1], accessor.PandasDelegate): | ||
| codes: np.ndarray = ... | ||
| codes: 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.
np_1d_array[np.intp]?
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.
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_nightly
| ) -> tuple[np.ndarray, np.ndarray]: ... | ||
| ) -> tuple[npt.NDArray[Any], 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 have no insight into this class / this class method, nor is it documented in any detail (personally I don't even know what "window bounds" means). It would be great if you suggest a few tests, or can we leave it in another PR?
|
|
||
| class CategoricalIndex(ExtensionIndex[S1], accessor.PandasDelegate): | ||
| codes: np.ndarray = ... | ||
| codes: 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.
| mid = _MidDescriptor() | ||
| length = _LengthDescriptor() |
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 fairly confused by these , aren't they just float and int respectively? not sure what these descriptors are
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 seems we cannot overload
property-ies - For
mid, it isfloatforintandfloat, butTimestampforTimestamp, andTimedeltaforTimedelta - For
length, it isfloatforfloat,intforint, butTimedeltaforTimestampandTimedelta
| @property | ||
| def is_monotonic_increasing(self) -> bool: ... | ||
| def clear_mapping(self) -> None: ... | ||
| class IntervalTree(IntervalMixin): ... |
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.
if the above comment is right, this can probably just be removed?
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.
class IntervalTree is something implemented in pandas with Cython. I don't understand what's happening with Ctyhon, but would like to leave its presence in the stubs, even though it's empty. Please tell me if you have a good argument to remove it completely.
pandas-stubs/core/base.pyi
Outdated
| | np_ndarray_float | ||
| | np_ndarray_complex | ||
| | dict[str, np.ndarray] | ||
| | dict[str, np_1darray[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.
the general pattern you seem to be following is:
- for function arguments, don't be restrictive on shape
- for return types, be as precise as possible
Given that NumListLike is often used as argument, would this one be better as Any shape?
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.
for function arguments, don't be restrictive on shape
This is relevant until the end of python 3.10 because it can only use an old version of numpy, which does not give the correct shape upon construction, i.e. np.array([1, 2, 3]) does not give a 1-d array with that old version, in static typing.
would this one be better as Any shape?
Yep dc85974
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_nightly
| mid = _MidDescriptor() | ||
| length = _LengthDescriptor() |
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 seems we cannot overload
property-ies - For
mid, it isfloatforintandfloat, butTimestampforTimestamp, andTimedeltaforTimedelta - For
length, it isfloatforfloat,intforint, butTimedeltaforTimestampandTimedelta
| @property | ||
| def is_monotonic_increasing(self) -> bool: ... | ||
| def clear_mapping(self) -> None: ... | ||
| class IntervalTree(IntervalMixin): ... |
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.
class IntervalTree is something implemented in pandas with Cython. I don't understand what's happening with Ctyhon, but would like to leave its presence in the stubs, even though it's empty. Please tell me if you have a good argument to remove it completely.
pandas-stubs/core/base.pyi
Outdated
| | np_ndarray_float | ||
| | np_ndarray_complex | ||
| | dict[str, np.ndarray] | ||
| | dict[str, np_1darray[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.
for function arguments, don't be restrictive on shape
This is relevant until the end of python 3.10 because it can only use an old version of numpy, which does not give the correct shape upon construction, i.e. np.array([1, 2, 3]) does not give a 1-d array with that old version, in static typing.
would this one be better as Any shape?
Yep dc85974
npt.NDArrayto benpt.NDArray[Any]