CLN: Assorted Cleanups #27791
CLN: Assorted Cleanups #27791
Conversation
jbrockmendel
commented
Aug 6, 2019
jbrockmendel
commented
- make PTA/DTA strftime() return ndarray instead of Index
- fix a xfailed Series[Period] test
- avoid imports of Index subclass in arrays.categorical and arrays.datetimelike
- some typing
|
Hello @jbrockmendel! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-08-07 00:56:59 UTC |
gfyoung
added
Clean
Internals
| def test_NaT_cast(self): | ||
| result = Series([np.nan]).astype("period[D]") | ||
| expected = Series([pd.NaT]) | ||
| expected = Series([pd.NaT], dtype="period[D]") |
There was a problem hiding this comment.
This is changing a test, no? Why are we dropping the xfail ?
There was a problem hiding this comment.
IIUC the xfail was from a time before dtype="period[D]" was recognized
|
|
||
| other = TimedeltaIndex(other) | ||
| other = TimedeltaArray._from_sequence(other) |
There was a problem hiding this comment.
if other is a timedelta64 dtype, the _from_sequence is not needed (and rather confusing the reader IMO)
There was a problem hiding this comment.
it is timedelta64, but not necessarily timedelta64[ns].
There was a problem hiding this comment.
@jbrockmendel I think we should avoid using _from_sequence for that in our code base. Maybe array(.., dtype='timedelta64[ns]) instead ?
There was a problem hiding this comment.
why do you want to avoid _from_sequence?
| if is_categorical(values): | ||
| values = CategoricalIndex(values) | ||
| # The CategoricalIndex level we want to build has the same categories | ||
| if is_categorical_dtype(values): |
There was a problem hiding this comment.
IIRC @jschendel didn't we just change this to make a perf fix? is this change compat with that
There was a problem hiding this comment.
I put together this version by looking at the old version and taking out the non-executed paths. So this shouldn't take a hti
There was a problem hiding this comment.
OT: we might be able to remove is_categorical entirely?
There was a problem hiding this comment.
Yah we should. Will need to be deprecated since its in the API
|
thanks @jbrockmendel |
