Skip to content

Add simple test for GH 28448 #29269

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 31, 2019
Merged

Add simple test for GH 28448 #29269

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6150b9
Add simple test for GH 28448
Oct 29, 2019
d829309
Update test so there is no np.asarray, but rather the original return…
Oct 30, 2019
0be007f
Write expected output in test more succinctly
Oct 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions pandas/tests/extension/test_categorical.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,28 @@ def test_cast_category_to_extension_dtype(self, expected):

tm.assert_series_equal(result, expected)

@pytest.mark.parametrize(
"data, dtype, expected",
[
(
"2015-01-01",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note - since data is always the same no need to parametrize

"datetime64[ns]",
np.array(["2015-01-01T00:00:00.000000000"], dtype="datetime64[ns]"),
),
(
"2015-01-01",
"datetime64[ns, MET]",
np.array(
[Timestamp("2015-01-01 00:00:00+0100", tz="MET")], dtype=object
),
),
],
)
def test_consistent_casting(self, data, dtype, expected):
# GH 28448
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OP uses asarray which is different than what is being tested here. Can you not do asarray in the test? The return type should be a date time array

Copy link
Member Author

@MarcoGorelli MarcoGorelli Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in, pandas.arrays.DatetimeArray? pd.Categorical(data).astype(dtype) is only of that type in the second case (when dtype is datetime64[ns, MET]).

In the first case (when dtype is datetime64[ns]), it is of type np.array.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm well I think that is a problem that as type would return a numpy array for "datetime64[ns]" and a DatetimeArray for "datetime64[ns, ]" but @jbrockmendel or @TomAugspurger might know more

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't totally follow the discussion but it seems like #28448 is about Categorical.astype(datetime64[ns, tz]), which return a DatatimeArray.

So I think the test should be something like

result = pd.Categorical(data).astype(dtype)
tm.assert_equal(result, expected)

no asarray.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right I think so too. But the problem is Categorical.astype("datetime64[ns]") returns a NumPy array, so tz-naive date times would be a numpy array where tz-aware would be a DTA.

That seems odd to me but I don't know the entire history of these

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory Categorical.astype('datetime64[ns]') could return a tz-naive DatetimeArray. I'm not sure if it was discussed explicitly when DatetimeArray was added, but changing that would be API-breaking and IMO not worth it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillAyd I tend to agree with everyone above in that this is sub-optimal behavior but not worth changing on its own*. Is this a problem or A Problem?

* There's been some discussion of making EA.astype always return EA at some point, which I think would include this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you guys don't think so then no. I just wasn't aware of the historical context hence the ping - thanks for the insights.

So @MarcoGorelli still should update the tests to use asarray but vary expectation accordingly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks all for your feedback - I've updated the tests

result = np.asarray(pd.Categorical(data).astype(dtype))
assert result == expected


class TestArithmeticOps(base.BaseArithmeticOpsTests):
def test_arith_series_with_scalar(self, data, all_arithmetic_operators):
Expand Down