CLN/REF: indexing typing, prune unreachable branches #27351
Conversation
WillAyd
left a comment
WillAyd
left a comment
There was a problem hiding this comment.
Some hopefully helpful comments. Generally annotating return types would be nice too
| _set_value.__doc__ = set_value.__doc__ | ||
|
|
||
| def _ixs(self, i, axis=0): | ||
| def _ixs(self, i: int, axis: int = 0): |
There was a problem hiding this comment.
Since axis is a pretty common keyword can you put that as Axis in pandas._typing? We probably also want to add str as a type for it since it should accept "index" and "columns" as well
There was a problem hiding this comment.
Also can we add the return type here? -> 'DataFrame'?
There was a problem hiding this comment.
I'll take a look and see what we can determine about return types. For Axis/str/int, in all these cases, I've put int because these are private methods and only ever get ints.
| if axis is None: | ||
| axis = self.axis or 0 | ||
|
|
||
| def _get_label(self, label, axis: int): |
| self.obj._data = self.obj._data.setitem(indexer=indexer, value=value) | ||
| self.obj._maybe_update_cacher(clear=True) | ||
|
|
||
| def _setitem_with_indexer_missing(self, indexer, value): |
There was a problem hiding this comment.
itd just be Any for both of em. Is that materially better than leaving it blank? In general I prefer to leave it blank rather than put a too-loose type that I'm not sure about
jreback
left a comment
jreback
left a comment
There was a problem hiding this comment.
looks good. some small changes. ping on green.
| def _slice(self, obj, axis=None, kind=None): | ||
| if axis is None: | ||
| axis = self.axis | ||
| def _slice(self, obj, axis: int, kind=None): |
| self.obj._maybe_update_cacher(clear=True) | ||
|
|
||
| def _setitem_with_indexer_missing(self, indexer, value): | ||
| """ |
There was a problem hiding this comment.
can you add a doc-string though
There was a problem hiding this comment.
I'd rather not. If I did, it would just say both parameters are object, which may be too loose. I'd rather leave it open for someone to reason about and add a more-correct version.
There was a problem hiding this comment.
actually I think spitting this into a ndim==1 and a ndim==2 functions would be helpful (future PR ok)
|
still would like to move to a unified pandas/core/indexing/..... structure sooner rather than later |
|
|
||
| maybe_shortcut = False | ||
| if hasattr(self, "columns") and isinstance(self.columns, MultiIndex): | ||
| if self.ndim == 2 and isinstance(self.columns, MultiIndex): |
There was a problem hiding this comment.
in the future change to use ABCMultiIndex
|
ok to merge, though if you want to split the setting routine I think might be helpful here. |
will do in next round. lots of balls (branches) in the air |
k |
|
thanks |
3 participants
No description provided.