Skip to content

Fix series.map overloads #960

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Fix series.map overloads #960

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3fe9674
Fix series.map overloads
JanEricNitschke Jul 13, 2024
7adb42d
Fix tests to properly exercise lack of 'pd.NA' support.
JanEricNitschke Jul 15, 2024
8810cc7
Further splitting of map overloads
JanEricNitschke Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pandas-stubs/core/series.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,7 +917,7 @@ class Series(IndexOpsMixin[S1], NDFrame):
def map(
self,
arg: Callable[[S1], S2 | NAType] | Mapping[S1, S2] | Series[S2],
na_action: Literal["ignore"] = ...,
na_action: Literal["ignore"],
) -> Series[S2]: ...
@overload
def map(
Expand Down
18 changes: 18 additions & 0 deletions tests/test_series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3276,18 +3276,36 @@ def test_map_na() -> None:

mapping = {1: "a", 2: "b", 3: "c"}
check(assert_type(s.map(mapping, na_action=None), "pd.Series[str]"), pd.Series, str)
check(assert_type(s.map(mapping), "pd.Series[str]"), pd.Series, str)

def callable(x: int | NAType) -> str | NAType:
if isinstance(x, int):
return str(x)
return x

def bad_callable(x: int) -> int:
return x << 1

with pytest.raises(TypeError):
s.map(
bad_callable, na_action=None # type: ignore[arg-type] # pyright: ignore[reportCallIssue, reportArgumentType]
)
with pytest.raises(TypeError):
s.map(bad_callable) # type: ignore[arg-type] # pyright: ignore[reportArgumentType]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From how we want to test the stubs, this is not a good test, and I think it should be deleted. What we want in the tests is the following:

  1. Tests of valid code that should pass the type checker (that's about 98% of our tests)
  2. Tests of invalid code that will raise exceptions that the type checker should catch. We surround such tests with if TYPE_CHECKING_INVALID_USAGE and then put the ignore tags on those tests, which also means that the code will not be tested at runtime.

In your case, bad_callable() has a signature that could be a "good" callable, and the type checker can't tell the difference. So I don't think the 2 tests you have here should be included. The test below is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the essence of these two tests is important. A callable that does not explicitly allow pd.NA as an argument should not be valid to be passed to Series.map if na_action=None (either explicitly or as a default value). The default value case was not detected before the changes that i made in this PR and caused a runtime error in a project of mine. So i do think it makes sense to explicitly test this case.

I do agree that they should be moved into a TPYE_CHECHKING_INVALID_USAGE block and have the pytest.raises removed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the essence of these two tests is important. A callable that does not explicitly allow pd.NA as an argument should not be valid to be passed to Series.map if na_action=None (either explicitly or as a default value). The default value case was not detected before the changes that i made in this PR and caused a runtime error in a project of mine. So i do think it makes sense to explicitly test this case.

You'd still have a problem for the people that do not use pd.NA yet. So if you had

def float_callable(x: float) -> float:
    return x + 1

Then if you had s = pd.Series([1.1, np.nan, 2.2]), then s.map(float_callable) is valid code. Your typing is preventing that.

Copy link
Contributor Author

@JanEricNitschke JanEricNitschke Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then i would argue that the hints are just poor and that is purely on the user (author of the function).

Take for example:

def add(a: float, b: float) -> float:
    return a+b

add(1.0, 2.0)
add("1", "2")

While both calls do not cause issues at run time, the second should definitely cause an error from the type checker. Because the type checker only validates the signature, because that is what external users interact with. The implementation could change at any point and cause runtime errors in the future if you are using the function in a way that does not match the signature.

even more explicitly

float_callable(pd.NA)

causes a typechecking failure by itself.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then i would argue that the hints are just poor and that is purely on the user (author of the function).

I agree. But we also make choices in the pandas stubs that cover the more common usage of pandas. Handling the case of np.nan versus pd.NA is a long topic of discussion in the pandas team, and we hope to resolve it some day. But right now, the majority of pandas users use np.nan to represent missing values, so we can't have stubs that prevent their code from working. The changes you are proposing cause existing (and valid) pandas code to fail.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if you want to separate the cases of na_action=None being explicit which should not be allowed, versus na_action=None being implicit (because None is the default value), I'd be OK with that. For me, what's important is that the implicit case should pass type checking, because the default behavior (not specifying na_action) is most likely what people use today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that is basically exactly what the status is before this PR actually, because it allows the interpretation of na_action="ignore" as a default value which is more forgiving. Then i would just add a TPYE_CHECKING_INVALID_USAGE for s.map(bad_callable, na_action=None) which captures the current behavior.

Additionally, with na_action=None (especially implicitly), the reason why a lot of current incorrect (pure type theory wise) code doesnt cause runtime errors despite not handling pd.NA and np.nan explicitly, is because they play nicely with most number interactions by just propagating them. However, this is not the case with most other types. So maybe it would make sense to also fail on the implicit case for functions that take most types, but explicitly allow int/float/complex?

Ill try to see if i can do it and add it to the PR to show what i mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the changes i proposed above and added some comments on the PR to explain them. Let me know what you think.

I actually also wanted to specifically allow mappings to return none-values Mapping[S1, S2] -> Mapping[S1, S2 | NAType] but this seems to mess with mypys overload resolution again.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with putting # type: ignore statements in the stubs when mypy and/or pyright complains about things like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but for mypy it also causes issues at the user side where it just infers things as Any.

check(
assert_type(s.map(bad_callable, na_action="ignore"), "pd.Series[int]"),
pd.Series,
int,
)

check(
assert_type(s.map(callable, na_action=None), "pd.Series[str]"), pd.Series, str
)
check(assert_type(s.map(callable), "pd.Series[str]"), pd.Series, str)

series = pd.Series(["a", "b", "c"])
check(assert_type(s.map(series, na_action=None), "pd.Series[str]"), pd.Series, str)
check(assert_type(s.map(series), "pd.Series[str]"), pd.Series, str)


def test_case_when() -> None:
Expand Down
Loading