Skip to content
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

ENH: Forward operators to underlying types #820

Closed
twoertwein opened this issue Nov 25, 2023 · 3 comments
Closed

ENH: Forward operators to underlying types #820

twoertwein opened this issue Nov 25, 2023 · 3 comments

Comments

@twoertwein
Copy link
Member

Describe the bug
Series is a container that forwards operators (+, -, ...) to the underlying types.

Some of the operators of Series could be better, for example, __add__ which:

  • returns Series[Unknown] instead of Series[return type]
  • or uses non-exiting helper classes to restrict the types

Instead of returning too wide types, enumerating all typing combinations, or creating non-existing helper classes, it might(?) be possible to abuse Protocols to forward operators to the underlying types.

To Reproduce
The following code works with both mypy and pyright

from __future__ import annotations
from typing import Protocol, TypeVar, overload, Generic

_T = TypeVar("_T", covariant=True)
_Other = TypeVar("_Other", contravariant=True)
_Result = TypeVar("_Result", covariant=True)

class add(Protocol[_Other, _Result]):
    def __add__(self, other: _Other, /) -> _Result:
        ...

class radd(Protocol[_Other, _Result]):
    def __radd__(self, other: _Other, /) -> _Result:
        ...

class Series(Generic[_T]):  # simulate Series with one element
    def __init__(self, x: _T) -> None:
        self.x = x

    # Series + Series
    @overload
    def __add__(
        self: Series[add[_Other, _Result]], other: Series[_Other], /
    ) -> Series[_Result]:
        ...

    @overload
    def __add__(self, other: Series[radd[_T, _Result]], /) -> Series[_Result]:
        ...

    # Series + scalar
    @overload
    def __add__(
        self: Series[add[_Other, _Result]], other: _Other, /
    ) -> Series[_Result]:
        ...

    @overload
    def __add__(self, other: radd[_T, _Result], /) -> Series[_Result]:
        ...

    def __add__(self, other):
        if isinstance(other, Series):
            return Series(self.x + other.x)
        return Series(self.x + other)

def test_int(x: Series[int], y: int):
    reveal_type(x + y)  # Series[int] (pandas-stubs: Series[int])

def test_float(x: Series[int], y: float):
    reveal_type(x + y)  # Series[float] (pandas-stubs: Series[Unknown])

def test_series_int(x: Series[int], y: Series[int]):
    reveal_type(x + y)  # Series[int] (pandas-stubs: Series[int])

def test_series_float(x: Series[int], y: Series[float]):
    reveal_type(x + y)  # Series[float] (pandas-stubs: Series[Unknown])

def test_error(x: Series[int], y: str):
    x + y  # error :) (pandas-stubs: no error)

def test_series_error(x: Series[int], y: Series[str]):
    x + y  # error :) (pandas-stubs: no error)
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Nov 25, 2023

This is a nice idea, but how you would handle the cases of Series[Timestamp].__add__(Series[Timedelta] ? Also, Series[Timestamp].__add__(Series[Timestamp] would need to be invalid, and I think your proposal wouldn't catch that??

@twoertwein
Copy link
Member Author

twoertwein commented Nov 26, 2023

Timestamp+Timedelta works, if I remove all the overloads but this one. Not sure why mypy/pyright are confused by that. With the overloads, both say there is no matching rule. Timedelta+Timestamp (the reverse) works without changes. I think mypy/pyright just pick the first overload (Timedelta.__add__'s relevant overload happens to be the first one but that is not the case for Timestamp.__add__)? I will open an issue at pyright (microsoft/pyright#6549)

Unrelated: _T being covariate might also cause troubles with how it would be used in the actual Series (the current TypeVar there is invariant).

@twoertwein
Copy link
Member Author

Closing, see microsoft/pyright#6549 (comment)

It is said that there is no generic approach to forward type annotations from encapsulated types. It underminds the usefulness of the Series dtype a bit (will be lost with many operators).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants