BUG: fix to_datetime failing to raise on mixed tznaive/tzaware datetimes #24663
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24663 +/- ##
==========================================
+ Coverage 92.37% 92.37% +<.01%
==========================================
Files 166 166
Lines 52382 52382
==========================================
+ Hits 48388 48389 +1
+ Misses 3994 3993 -1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24663 +/- ##
==========================================
+ Coverage 92.37% 92.37% +<.01%
==========================================
Files 166 166
Lines 52382 52382
==========================================
+ Hits 48388 48389 +1
+ Misses 3994 3993 -1
Continue to review full report at Codecov.
|
| @@ -16,6 +16,25 @@ | |||
|
|
|||
|
|
|||
| class TestDatetimeArrayConstructor(object): | |||
| def test_mixing_naive_tzaware_raises(self): | |||
| # GH#24569 | |||
| arr = np.array([pd.Timestamp('2000'), pd.Timestamp('2000', tz='CET')]) | |||
There was a problem hiding this comment.
Is there a test that verifies mixed tz example from the issue raises? I didn't see one quickly glancing over this file, but could be missing it or it could be elsewhere. If not, you could maybe parametrize over a data variable that does both orders for aware/naive and the aware/aware case and get ride of the for obj loop, i.e.
@pytest.mark.parametrize('data', [
[pd.Timestamp('2000', tz='CET'), pd.Timestamp('2000')],
[pd.Timestamp('2000'), pd.Timestamp('2000', tz='CET')],
[pd.Timestamp('2000', tz="US/Central"), pd.Timestamp('2000', tz='CET')]])
def test_mixing_naive_tzaware_raises(self):
arr = np.array(data)There was a problem hiding this comment.
Or alternatively parametrizing over timezone pairs if it seems cleaner, e.g.
@pytest.mark.parametrize('tz1, 'tz2', [("CET", None), (None, "CET"), ("US/Central", "CET")])
There was a problem hiding this comment.
I think this does does hit the case from this original issue, in the DatetimeArray._from_sequence case.
There was a problem hiding this comment.
Yes, agreed that the DatetimeArray._from_sequence case hits the original issue.
I think my comment was a bit unclear; what I was asking is if we have coverage for the working mixed tz example you also provided in the issue: mix = np.array([pd.Timestamp('2000', tz="US/Central"), pd.Timestamp('2000', tz='CET')]). I didn't see a test for that anywhere, and figured we could shoehorn it into this test if it's indeed not covered.
There was a problem hiding this comment.
Not a pressing issue or anything though, so if it's not covered it we could just make it a follow-up item; don't want this to block other work.
| @@ -167,6 +167,7 @@ def datetime_to_datetime64(values: object[:]): | |||
| int64_t[:] iresult | |||
| npy_datetimestruct dts | |||
| _TSObject _ts | |||
| bint found_naive = False | |||
There was a problem hiding this comment.
@jbrockmendel is there a reason we cannot remove this routine entirely in favor of using array_to_datetime? can you create an issue about this (though maybe you already did)
There was a problem hiding this comment.
#24006 does this, but is on hold ATM
|
This is good to go? Do the updates to the tests satisfy |
|
thanks @jbrockmendel |
|
@jschendel I think your concerns are satisfied but if not pls raise an issue / PR. |
git diff upstream/master -u -- "*.py" | flake8 --diff