Skip to content

REF: avoid importing pd.NA in tslibs #34009

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
merged 5 commits into from
May 9, 2020
Merged

Conversation

jbrockmendel
Copy link
Member

The existing cimport of pd.NA into tslibs.nattype is a circular dependency (its not actually clear to me why it doesnt raise). I'm finding that its presence is making other refactoring around tslibs.offsets more difficult. This PR moves the import of pd.NA to tslib, breaking the circular dependency.

This does change some behavior, but none that is tested, so we should discuss what places we want to recognize pd.NA. In particular, pd.Timedelta(pd.NA) returns NaT in master and raises ValueError in this PR. pd.Timestamp(pd.NA) and pd.Period(pd.NA) raise TypeError and ValueError, respectively, in both master and this PR.

cc @jorisvandenbossche what is your preferred behavior?

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented May 6, 2020

This keeps importing pd.NA into tslibs, though? (but in tslibs.pyx itself it is less a problem?)

The behaviour change is certainly fine for me. I think it is even better than we raise for pd.NA for now (since pd.NA is not yet used in any datetimelike dtype.

@jbrockmendel
Copy link
Member Author

This keeps importing pd.NA into tslibs, though? (but in tslibs.pyx itself it is less a problem?)

tslib.pyx, but not the tslibs directory, right. Since nothing inside tslibs/ depends on tslib.pyx, it is not that big a deal.

@jorisvandenbossche
Copy link
Member

Ah, I missed the tslib.pyx vs tslib directory

@jreback jreback added Clean Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels May 6, 2020
@jreback jreback added this to the 1.1 milestone May 6, 2020
@jreback
Copy link
Contributor

jreback commented May 6, 2020

can you merge master. do we have sufficient test for this change?

@jorisvandenbossche
Copy link
Member

We could in principle test pd.Timedelta(pd.NA), but I also don't think that's very important, as I would regard that behaviour experimental / subject to change.

@jreback jreback merged commit 9518523 into pandas-dev:master May 9, 2020
@jbrockmendel jbrockmendel deleted the missing branch May 9, 2020 21:43
rhshadrach pushed a commit to rhshadrach/pandas that referenced this pull request May 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants