TYP: setter for index/columns property-like (AxisProperty) #46565
Conversation
| DataFrame, | ||
| Index, | ||
| Series, |
There was a problem hiding this comment.
There is an issue with mypy that can't resolve the types for the decorated methods that IIRC @jbrockmendel encountered before when adding the stubs for the Cython code initially. It seems to be related to the imports and putting them here seems prevent the problem resurfacing.
| @overload | ||
| def __get__(self, obj: None, type) -> AxisProperty: ... | ||
| def __set__( | ||
| self, obj: DataFrame | Series, value: AnyArrayLike | Sequence |
There was a problem hiding this comment.
type annotation for value chosen just to match _set_axis which is added to Series to match the existing ensure_index annotation and changed on NDFrame for consistency.
There was a problem hiding this comment.
Just an idea, might be not worth it to enforce that obj is compatible with value.
IndexCompatibleT = TypeVar("IndexCompatibleT", AnyArrayLike, Sequence)
class Has_set_axis(Protocol[IndexCompatibleT]):
def _set_axis(self: NDFrame, axis: int, value: IndexCompatibleT) -> object:
...
def __set__(self, obj: Has_set_axis[IndexCompatibleT], value: IndexCompatibleT) -> None:
...There was a problem hiding this comment.
from the other response, I think that only NDFrame objects use AxisProperty and we tend to avoid NDFrame in annotations that would be part of the public API since NDFrame is not public, but obviously need to use it in some circumstances.
Until the other issue is resolved, I'm not sure I can comment on whether we would be better off using a protocol here instead.
There was a problem hiding this comment.
to be fair, obj would probably not surface in the public api, only value would.
There was a problem hiding this comment.
the index/column properties are only actually added to DataFrame/Series in the codebase.
In theory they could be added to any NDFrame object and I suppose we could refactor the code to add the AxisProperty dynamically in the base class based on the other NDFrame attributes, i.e. we know the axis numbers and names.
Having said that, i'm -1 on doing this dynamically and therefore happy with the DataFrame|Series to match the actual code.
| if not isinstance(result, (ABCSeries, ABCDataFrame)) or not result.index.equals( | ||
| obj.index | ||
| obj.index # type:ignore[arg-type] |
There was a problem hiding this comment.
not investigated this one yet. maybe fix later.
There was a problem hiding this comment.
Is AxisProperty used by all NDFrame? That might cover most/all objects in this union.
A slightly (too) complex approach would be to use a Protocol for __get__: only objects that have _mgr and only _mgr that have _mgr.axes where axes is Sequence[Index].
There was a problem hiding this comment.
I'm not sure that all those objects are valid e.g. AttributeError: 'DataFrameGroupBy' object has no attribute 'index'
I assume that the condition should be narrowing to DataFrame/Series from the error message.
| # "Optional[List[str]]", variable has type "Index") | ||
| ret.columns = columns # type: ignore[assignment] | ||
| # columns is not narrowed by mypy from relabeling flag | ||
| assert columns is not None # for mypy |
There was a problem hiding this comment.
not yet ran the tests locally to check we don't get failures but it appears relabeling and columns are not independent variables, which mypy cannot understand and we should try to avoid. Not planning to refactor at this time.
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
…v#46565) Co-authored-by: Matthew Roeschke <[email protected]>
No description provided.