Refactor _TimeOp._validate to separate datetime vs timedelta vs dateoffset #18832
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18832 +/- ##
==========================================
- Coverage 91.64% 91.63% -0.02%
==========================================
Files 154 154
Lines 51408 51387 -21
==========================================
- Hits 47113 47088 -25
- Misses 4295 4299 +4
Continue to review full report at Codecov.
|
gfyoung
added
Clean
Internals
pandas/core/ops.py
Outdated
| if ((self.is_integer_lhs or self.is_floating_lhs) and | ||
| self.is_timedelta_rhs): | ||
|
|
||
| if name not in ('__div__', '__truediv__', '__mul__', '__rmul__'): |
There was a problem hiding this comment.
maybe factor this to a function to avoid duplication with above,
assert_can_operate() or somesuch
| @@ -960,8 +960,44 @@ def test_timedelta64_ops_nat(self): | |||
| assert_series_equal(timedelta_series / nan, | |||
| nat_series_dtype_timedelta) | |||
|
|
|||
| def test_operators_timedelta64_with_timedelta(self): | |||
| # smoke tests | |||
| td1 = Series([timedelta(minutes=5, seconds=3)] * 3) | |||
There was a problem hiding this comment.
we certainly have some of these, can you remove the existing ones if you are consolidating.
these also seem oddly specific, would parametrize on Timedelta, TDI as well here
There was a problem hiding this comment.
we certainly have some of these, can you remove the existing ones if you are consolidating.
Yes, these are already moved out of the giant .test_operators_datetimelike
these also seem oddly specific, would parametrize on Timedelta, TDI as well here
Not sure about TDI, but definitely can parametrize over scalar timedelta variants.
|
|
||
| def test_operators_timedelta64_with_timedelta_invalid(self): | ||
| td1 = Series([timedelta(minutes=5, seconds=3)] * 3) | ||
| td1.iloc[2] = np.nan |
| @@ -976,16 +1012,6 @@ def run_ops(ops, get_ser, test_ser): | |||
| # ## timedelta64 ### | |||
There was a problem hiding this comment.
is this initialization code then used below?
| return self._validate_datetime(lvalues, rvalues, name) | ||
| elif self.is_timedelta_lhs: | ||
| return self._validate_timedelta(name) | ||
| elif self.is_offset_lhs: |
There was a problem hiding this comment.
I guess this is ok for now. Maybe should make 3 sub-classes here (or I think ultimately refactoring to have all this logic in the index methods as it already duplicates some of this).
|
thanks! |
Trying to mirror the logic in the datetimelike index classes.
Separate some of the tests in the same vein.
No logic should be changed in this PR, it should be pure refactor.
This will have a merge conflict with #18831.