Fix series.map overloads #960

JanEricNitschke · 2024-07-13T09:27:24Z

Tests added: Please use assert_type() to assert the type of any return value

In my last PR that handles series.map() i introduced a slight error by adding the = ... to the overload with "ignore" as this is not the actual default value.

Subsequently invalid function that do not handle pd.NA were accepted when the na_action was not explicitely specified.

This has been addressed and test cases have been added to check this.

Dr-Irv · 2024-07-15T14:05:29Z

tests/test_series.py

+    s.map(
+        bad_callable, na_action=None  # type: ignore[arg-type] # pyright: ignore[reportCallIssue, reportArgumentType]
+    )
+    s.map(bad_callable)  # type: ignore[arg-type] # pyright: ignore[reportArgumentType]


The type checkers should accept those patterns, because at least in my tests at runtime, no error is raised. So why are you considering that a "bad callable"?

Ah, may bad. So normally if na_action=None then the function has to be able to handle pd.NA because it gets passed in. And for my bad_callable it does not explicitely handle it, at least according to the type hint.

I just chose a poor operation, to perform in the function, because pd.NA supports addition of an integer.

I have now changed it for a different operation on integers that pd.NA does not incidentally support.

Dr-Irv · 2024-07-15T14:43:06Z

tests/test_series.py

+    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]


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:

Tests of valid code that should pass the type checker (that's about 98% of our tests)

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.

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.

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.

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.

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.

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.

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.

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.

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

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

JanEricNitschke · 2024-07-16T16:21:33Z

tests/test_series.py

+
+    s2: pd.Series[float] = pd.Series([1.0, pd.NA, 3.0])
+
+    def callable2(x: float) -> float:


Float (and int, complex, bool) callables pass na_action="ignore" and default na_action despite no explicit None handling, but fail the explicit na_action=None case as it is assumed that this is explicitly specified for rigorous type checking.

JanEricNitschke · 2024-07-16T16:23:18Z

tests/test_series.py

+
+    s3: pd.Series[str] = pd.Series(["A", pd.NA, "C"])
+
+    def callable3(x: str) -> str:


Non-numerical functions pass only the explicit na_action="ignore" but not the na_action=None case (explicit or implicit) because these are likely to cause runtime errors on nan-values.

Dr-Irv · 2024-07-16T20:31:27Z

tests/test_series.py


    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)
+
+    s2: pd.Series[float] = pd.Series([1.0, pd.NA, 3.0])


We generally don't use the annotations in the tests. So you can do

s2 = pd.Series([1.0, pd.NA, 3.0], dtype=float) # OR s2 = pd.Series([1.0, pd.NA, 3.0]).astype(pd.Float64Dtype())

Then the type checkers should recognize s2 as Series[float]

Note that internally, pd.Series([1.0, pd.NA, 3.0]) has dtype object

This first actually gives me an error TypeError: float() argument must be a string or a real number, not 'NAType'. While the second version does work, i cant get it to work for the integer case of s = pd.Series([1, pd.NA, 3]). While that does work at the construction point and for inferring s: Series[int], the values that are passed into callable at s.map(callable) are actually floats.

For Series[int], you can use s = pd.Series([1, pd.NA, 2], dtype="Int64")

Sorry about my error on the dtype=float.

# Step 1: Create the Series with pd.NA s = pd.Series([1, pd.NA, 3], dtype="Int64") print(s) print(s.dtype) # Define the callable function def callable(x: int | NAType) -> str | NAType: print(x, type(x)) if pd.isna(x): return pd.NA return str(x) print(s) print(s.map(callable, na_action=None))

actually still gives me this:

$ python testing.py 0 1 1 <NA> 2 3 dtype: Int64 Int64 0 1 1 <NA> 2 3 dtype: Int64 1.0 <class 'float'> nan <class 'float'> 3.0 <class 'float'> 0 1.0 1 <NA> 2 3.0 dtype: object

That's a known pandas issue. See pandas-dev/pandas#57189

Dr-Irv · 2024-07-16T20:32:03Z

tests/test_series.py

+    if TYPE_CHECKING_INVALID_USAGE:
+        s2.map(callable2, na_action=None)  # type: ignore[arg-type] # pyright: ignore[reportCallIssue, reportArgumentType]
+
+    s3: pd.Series[str] = pd.Series(["A", pd.NA, "C"])


Similar comment. Use s3 = pd.Series(["A", pd.NA, "C"], dtype=str)

Dr-Irv · 2024-07-16T20:33:55Z

tests/test_series.py

+        s3.map(
+            callable3, na_action=None  # type: ignore[arg-type] # pyright: ignore[reportCallIssue, reportArgumentType]
+        )
+        s3.map(callable3)  # type: ignore[type-var] # pyright: ignore[reportCallIssue, reportArgumentType]


I'm a little concerned that this won't pass the type checker. A user should be able to do:

s = pd.Series({"a", "b", "c"], dtype=str) s.map(callable3)

That will execute correctly and should pass the type checker.

Unfortunately i dont think the type system supports something like this at the moment.

All that s.map(callable3) sees is that the entries of s are string or none values and that none-values will be passed into callable3 as they are. So it has to be able to handle them. The type system does not know if there actually are non-values in there or not, only that there might be. Most of the times the user probably wont know either if the data is loaded from an external file that comes from a sensor or a data repository.

If the user does know more than the type system he could either specify na_action="ignore", which he should do anyway if the function does no handle the case explicitly, or add a type ignore to the line as one normally does when you have more information than can be contained in the type system (or understood by the type checkers).

I understand your point of view. But the issue here is that we need to support the "normal" usage of pandas. The type system can't keep track of what is inside of a Series or DataFrame. For example, if you have a DataFrame df with columns "a", "b" then df["a"] is Series[Any] because we can't track the type inside the DataFrame.

The default of the na_action argument in Series.map() is None, which means that people who know that their series has no non-null elements would now have to handle the case in the declared typing of callable3() to handle nulls, but that will likely break existing code.

We've had other cases where we were too restrictive in the stubs, and people complained, so we had to pull back.

See https://github.com/pandas-dev/pandas-stubs/blob/main/docs/philosophy.md#narrow-vs-wide-arguments for a discussion of this issue. IMHO, you're proposing that the stubs will be too narrow.

Yeah, i think im just too much of a typing purist in this case. Because, just because your code doesnt cause runtime errors with the data that you are testing it with, doesnt mean that it isnt unsound from a typing perspective. Normally every interaction should be proveably valid just from the involved signatures. And i am using a type checker to actually ensure this.

But i guess that is not fully possible anyway with the setup of pandas.

Yes, I agree. We've chosen a middle ground with the stubs that supports the common ways people use pandas, but doesn't necessarily support all possible best typing practices.

Dr-Irv · 2024-09-07T16:11:12Z

@JanEricNitschke can you address my comments at

I'll then have to take another careful look and see if there is anything else that I'm not comfortable with.

JanEricNitschke · 2024-09-08T04:59:41Z

@Dr-Irv ah, sorry, i had other life stuff come up.

I think it is not easily possible to make the changes i intended while keeping with the general philosophy of not disallowing things with the types that do happen to work at runtime.

So i would probably close this PR for now and maybe have another look again later when i have more time to see if there is still an option?

Does that work for you?

Dr-Irv · 2024-09-09T13:35:45Z

So i would probably close this PR for now and maybe have another look again later when i have more time to see if there is still an option?

Does that work for you?

Sure. I'll let you do the closing

Fix series.map overloads

3fe9674

Dr-Irv requested changes Jul 15, 2024

View reviewed changes

Fix tests to properly exercise lack of 'pd.NA' support.

7adb42d

Dr-Irv requested changes Jul 15, 2024

View reviewed changes

Further splitting of map overloads

8810cc7

JanEricNitschke commented Jul 16, 2024

View reviewed changes

Dr-Irv requested changes Jul 16, 2024

View reviewed changes

JanEricNitschke closed this Sep 9, 2024


		s2: pd.Series[float] = pd.Series([1.0, pd.NA, 3.0])

		def callable2(x: float) -> float:


		s3: pd.Series[str] = pd.Series(["A", pd.NA, "C"])

		def callable3(x: str) -> str:

Uh oh!

Fix series.map overloads #960

Fix series.map overloads #960

Uh oh!

Conversation

JanEricNitschke commented Jul 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanEricNitschke Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanEricNitschke Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dr-Irv commented Sep 7, 2024

Uh oh!

JanEricNitschke commented Sep 8, 2024

Uh oh!

Dr-Irv commented Sep 9, 2024

Uh oh!

Uh oh!

JanEricNitschke Jul 15, 2024 •

edited

Loading

JanEricNitschke Jul 16, 2024 •

edited

Loading