API: Replace na_action parameter in Series/DataFrame/Index.map by the standard skipna #61530
Conversation
| s = Series([1, 2, 3]) | ||
| msg = f"na_action must either be 'ignore' or None, {input_na_action} was passed" | ||
| with pytest.raises(ValueError, match=msg): | ||
| s.map({1: 2}, na_action=input_na_action) |
There was a problem hiding this comment.
These are already tested for DataFrame.map, Series.map and Index.map, together with the warning now. I didn't think it was worth testing the same here again only for Series.
| @@ -10719,16 +10725,28 @@ def map( | |||
| 0 1.000000 4.494400 | |||
| 1 11.262736 20.857489 | |||
| """ | |||
| if na_action not in {"ignore", None}: | |||
| raise ValueError(f"na_action must be 'ignore' or None. Got {na_action!r}") | |||
| if na_action != lib.no_default: | |||
There was a problem hiding this comment.
In case it's not obvious, I'm using lib.no_default as a sentinel to know if na_action was set by the user, so we can show them a warning. If I leave the default to None I can't tell if the user call was .map(func) (no warning needed) or .map(func, na_action=None) (warning needed)
| """ | ||
| return request.param | ||
|
|
||
|
|
There was a problem hiding this comment.
The skipna fixture already exists.
| @@ -2541,17 +2541,17 @@ def __array_ufunc__(self, ufunc: np.ufunc, method: str, *inputs, **kwargs): | |||
|
|
|||
| return arraylike.default_array_ufunc(self, ufunc, method, *inputs, **kwargs) | |||
|
|
|||
| def map(self, mapper, na_action: Literal["ignore"] | None = None): | |||
| def map(self, mapper, skipna: bool = False): | |||
There was a problem hiding this comment.
This is technically a public API since EA authors might have implemented map on their arrays
There was a problem hiding this comment.
True, I thought about it, but I couldn't find any extension array that implements map, at least in the ones in our ecosystem. And if there is one I'm not aware of, it should call super in order for this to affect them.
I'm ok to add a DeprecationWarning if you think it's better. I thought that in the unlikely case this breaks any code, we may get a report before the release, and see with a specific case what makes more sense here.
I addressed the other comments, thanks for the feedback.
There was a problem hiding this comment.
OK if you surveyed other EAs in the ecosystem, I feel more comfortable making this a breaking change. Could you note it in the breaking changes section at least in the v3.0.0.rst whatsnew?
There was a problem hiding this comment.
I was double checking. pandas-pint does implement map, but it doesn't call super, so this is not used. What I didn't see before is that it does call map_array, which is also having this change.
I'm still not sure if the warning is the best strategy, maybe I'm just overthinking.
@MichaelTiemannOSC, it would be good to have your feedback about this.
pandas/core/frame.py
Outdated
| skipna: bool = False, | ||
| na_action: Literal["ignore"] | None | lib.NoDefault = lib.no_default, |
There was a problem hiding this comment.
I think these should be switched to not break users passing arguments by position
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.We've been consistently using a boolean
skipnaparameter to let users decide whether missing values should be ignored or not in different methods. The.mapmethod has an awkwardna_action=None | "igonre"for the same purpose. I add the standardskipnaparameter to the methods, and start the deprecation of thena_actionone.