BUG: Series[timdelta64].var() should _not_ work #28289
Conversation
pandas/core/series.py
Outdated
| @@ -4012,6 +4012,13 @@ def _reduce( | |||
| If we have an ndarray as a value, then simply perform the operation, | |||
| otherwise delegate to the object. | |||
| """ | |||
| if name == "var" and is_timedelta64_dtype(self.dtype): | |||
| raise TypeError( | |||
There was a problem hiding this comment.
is there anyway you can do this in nanops instead? it is very awkward to do here
There was a problem hiding this comment.
Looks like this is no longer necessary, removed.
|
conflict, plus a comment above |
pandas/core/nanops.py
Outdated
| if is_timedelta64_dtype(values.dtype): | ||
| # std is well-behaved, but var is not | ||
| if mask is None: | ||
| mask = isna(values) |
There was a problem hiding this comment.
i think this might be better fixed if we have nanvar call _get_values (which already does this masking & type conversion); its the only routine which doesn't I think (well maybe sem).
There was a problem hiding this comment.
The problem with that is that we need different behavior for nanstd vs nanvar
There was a problem hiding this comment.
can you try to address this
There was a problem hiding this comment.
Well, no. We have @disallow("M8", "m8") on nanvar, so if we don't do this here, we'll raise as soon as we call nanvar with non-casted values.
There was a problem hiding this comment.
I am concerned about this code duplicatio of _get_values.
There was a problem hiding this comment.
Yep, turns out we can use _get_values. updated
|
gentle ping. more reductions to implement after this |
|
I don't think my comments above were addressed. the code change is duplicative, if you can find a way to combine with the existing routines. |
I replied to your comment suggesting that nanstd should go through nanvar: that is not a viable option. |
| @@ -300,11 +300,14 @@ def test_timedelta_ops(self): | |||
| assert result[0] == expected | |||
|
|
|||
| # invalid ops | |||
| for op in ["skew", "kurt", "sem", "prod"]: | |||
| for op in ["skew", "kurt", "sem", "prod", "var"]: | |||
There was a problem hiding this comment.
let's split this to a separate test that is paramterized (followon)
|
thanks |
* follow-up to pandas-dev#29314 * followup to pandas-dev#28289
* follow-up to pandas-dev#29314 * followup to pandas-dev#28289
* follow-up to pandas-dev#29314 * followup to pandas-dev#28289
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff