Skip to content

API: dont-special-case datetimelike in setting new column #40084

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

Merged

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • Ensure all linting tests pass, see here for how to run them
  • whatsnew entry

@jorisvandenbossche
Copy link
Member

What's the change in behaviour exactly? It will no longer infer datetime, or will no longer only infer datetimes?

(I mentioned that before, but I would find it helpful if you can generally provide a bit more context when opening PRs)

@jbrockmendel
Copy link
Member Author

(I mentioned that before, but I would find it helpful if you can generally provide a bit more context when opening PRs)

I generally prefer brevity in cases where the title is descriptive and the code change is small: one line of code and 1 affected test

@jreback jreback added the Dtype Conversions Unexpected or buggy dtype conversions label Feb 26, 2021
@jorisvandenbossche
Copy link
Member

I generally prefer brevity in cases where the title is descriptive and the code change is small: one line of code and 1 affected test

"descriptive" for you (who has been doing the change and thus of course understands what it is about) is something else as descriptive enough for someone else who is not you ;)
To be clear: the reason I gave this comment was exactly because the title was not descriptive for me. Without diving into the code myself, I don't know what special case you are talking about, or what it means in practice to no longer special case something.

It's not because it is only a 1 line change that it is necessarily easy to understand what it actually changes. To know that, I would need to know by heart all the possible inferences that Series does, vs what the maybe_infer_to_datetimelike function does. And no, I don't know this.

@jbrockmendel
Copy link
Member Author

possible inferences that Series does, vs what the maybe_infer_to_datetimelike function does. And no, I don't know this.

So big picture, the goal of this and other recent/upcoming PRs is to make this clearer, or at least less. In this case by removing usages of maybe_infer_to_datetimelike so we have fewer (ideally just one) flavor of casting.

maybe_infer_to_datetimelike(object_array_of_datetimes) will cast to dt64, but will not cast for object_array_of_intervals. As a result, df[newcol] = object_array_of_intervals stays as object dtype. This change makes df[newcol] = object_array_of_intervals infer to IntervalDtype.

@jreback jreback added this to the 1.3 milestone Feb 27, 2021
@jreback jreback merged commit 6fcc188 into pandas-dev:master Feb 27, 2021
@jbrockmendel jbrockmendel deleted the ref-maybe_infer_to_datetimelike branch March 31, 2021 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dtype Conversions Unexpected or buggy dtype conversions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants