-
-
Notifications
You must be signed in to change notification settings - Fork 18.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
DEPR: Remove NumericIndex #51139
DEPR: Remove NumericIndex #51139
Conversation
@@ -5375,6 +5362,7 @@ def identical(self, other) -> bool: | |||
for c in self._comparables | |||
) | |||
and type(self) == type(other) | |||
and self.dtype == other.dtype |
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.
Previously the index type could differentiate e.g. Index([1, 2, 3], dtype=object)
and Index([1, 2, 3], dtype="int64")
(the latter actually being a NumericIndex
), but we now need to be more precise.
@@ -539,7 +538,6 @@ def __new__( | |||
|
|||
klass = cls._dtype_to_subclass(arr.dtype) | |||
|
|||
# _ensure_array _may_ be unnecessary once NumericIndex etc are gone | |||
arr = klass._ensure_array(arr, arr.dtype, copy=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.
It's not unnecessary?
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 had to remove “NumericIndex” and had to decide what to do with comment and opted to delete it. IMO _ensure_array could be refactored, but so could a lot of other things in pandas and _ensure_array is not that bad anymore compared to other things, so doesn’t merit a comment In the code base IMO.
This was my reasoning, others could of course have made a different judgement call…So it’s not a strong opinion, and I’m fine with it being refactored, but also it not being refactored :-)
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.
Yeah no worries, just wasn't sure if this was still being True or not
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.
small comment (can also be addressed in a follow up), otherwise lgtm
thx @topper-123 |
This removal is the partially the cause of a regression unpickling files from "pandas<2", #53300 |
This remove action caused a bug |
This finishes the removal of
NumericIndex
from the code base. Note that this PR builds atop of #51132, so you might want to merge that first.Everything should be finished now after this PR, e.g. the doc updates have been merged (#51111) and the doc string has been updated (#51089). I'll have a final look once this is merged, but this should pretty much finish this issue.