-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API: tighten DTA/TDA _from_sequence signature #37179
Changes from 13 commits
eab1030
137fdf1
e54ed75
3d2a7ab
96a8475
d49d819
f693ed4
058338a
77e7c21
7e97d03
f2a2aaf
00d19e3
bc532be
2e612c4
b8db5c1
cad1104
8f1b25d
fb41950
9f72ad8
54c87da
d87533b
158f2b2
b071b5f
bffacb1
0153a51
5acede8
3b25043
8c87760
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,7 +43,12 @@ | |
pandas_dtype, | ||
) | ||
from pandas.core.dtypes.dtypes import DatetimeTZDtype | ||
from pandas.core.dtypes.generic import ABCIndexClass, ABCPandasArray, ABCSeries | ||
from pandas.core.dtypes.generic import ( | ||
ABCIndexClass, | ||
ABCMultiIndex, | ||
ABCPandasArray, | ||
ABCSeries, | ||
) | ||
from pandas.core.dtypes.missing import isna | ||
|
||
from pandas.core.algorithms import checked_add_with_arr | ||
|
@@ -302,6 +307,34 @@ def _simple_new( | |
|
||
@classmethod | ||
def _from_sequence(cls, scalars, dtype=None, copy: bool = False): | ||
|
||
scalars, copy = dtl.ensure_arraylike(scalars, copy) | ||
|
||
if scalars.dtype.kind == "M": | ||
pass | ||
elif scalars.dtype == object: | ||
if isinstance(scalars, ABCMultiIndex): | ||
raise TypeError("Cannot create a DatetimeArray from MultiIndex") | ||
|
||
inferred = lib.infer_dtype(scalars) | ||
if inferred in ["datetime64", "date", "datetime", "empty"]: | ||
pass | ||
else: | ||
msg = f"{inferred} scalars cannot be converted to datetime64[ns]" | ||
raise TypeError(msg) | ||
elif is_string_dtype(scalars.dtype): | ||
# TODO: should go through from_sequence_of_strings? | ||
pass | ||
elif ( | ||
is_categorical_dtype(scalars.dtype) and scalars.categories.dtype.kind == "M" | ||
): | ||
# TODO: Could also use Categorical[object] | ||
# with inferred_type as above? | ||
pass | ||
else: | ||
msg = f"dtype {scalars.dtype} cannot be converted to datetime64[ns]" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you ensure we have a test that hits this |
||
raise TypeError(msg) | ||
|
||
return cls._from_sequence_not_strict(scalars, dtype=dtype, copy=copy) | ||
|
||
@classmethod | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -224,6 +224,22 @@ def _from_sequence( | |
if dtype: | ||
_validate_td64_dtype(dtype) | ||
|
||
data, copy = dtl.ensure_arraylike(data, copy) | ||
|
||
if data.dtype.kind == "m": | ||
pass | ||
elif data.dtype == object: | ||
inferred = lib.infer_dtype(data) | ||
if inferred in ["timedelta64", "timedelta", "empty"]: | ||
pass | ||
else: | ||
raise TypeError(inferred) | ||
jbrockmendel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
elif is_string_dtype(data.dtype): | ||
# TODO: should go through from_sequence_of_strings? | ||
pass | ||
jbrockmendel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
else: | ||
raise TypeError(data.dtype) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you ensure we have a test that hits hits |
||
|
||
data, inferred_freq = sequence_to_td64ns(data, copy=copy, unit=None) | ||
freq, _ = dtl.validate_inferred_freq(None, inferred_freq, False) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2869,6 +2869,7 @@ def test_from_tzaware_mixed_object_array(self): | |
] | ||
assert (res.dtypes == expected_dtypes).all() | ||
|
||
@pytest.mark.xfail(reason="DatetimeArray._from_sequence no longer accepts i8") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need a note for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the question is if we want to disable/deprecate this or find a way to make it work There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this something we want to support or disallow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be clear, the question is not so much about
works fine (and the same for the other two). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tend to agree. Any thoughts on where to catch this case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not fully sure what you mean, but (until #33254 is resolved) doing such conversions is the responsibility of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I mean that we could make this Series construction work without loosening the _from_sequence restrictions by and instead calling [??] in this case.
To be clear, is this what you are suggesting we do? My impression from previous conversations is that you are unambiguously in favor of this tightening. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My earlier suggestion at #37179 (comment) was to use
Yes, for sure. But before we can implement that, we still need to come up with an API for the non-strict conversion as well, as that is still something we need to support. That's the whole discussion of #33254 |
||
def test_from_2d_ndarray_with_dtype(self): | ||
# GH#12513 | ||
array_dim2 = np.arange(10).reshape((5, 2)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this tested?