CLN: some code cleanups #32177
CLN: some code cleanups #32177
Conversation
| @@ -481,8 +481,7 @@ cdef class Interval(IntervalMixin): | |||
|
|
|||
| @cython.wraparound(False) | |||
| @cython.boundscheck(False) | |||
| def intervals_to_interval_bounds(ndarray intervals, | |||
| bint validate_closed=True): | |||
| def intervals_to_interval_bounds(ndarray intervals, bint validate_closed=True): | |||
There was a problem hiding this comment.
its worth double-checking me on this, but i think for non-cdef/cpdef functions, using cython syntax for the annotations doesnt help perf. in these cases, let's just use python versions
There was a problem hiding this comment.
right, but that is orthogonal to this PR
pandas/_libs/tslib.pyx
Outdated
| If datetime, convert to datetime.datetime | ||
| If date, convert to datetime.date | ||
| If time, convert to datetime.time | ||
| If Timestamp, convert to pandas.Timestamp |
There was a problem hiding this comment.
do we usually use dashes or asterisks or something for this kind of bullet point thing?
pandas/_libs/tslib.pyx
Outdated
|
|
||
| Returns | ||
| ------- | ||
| result : array of dtype specified by box | ||
| array of dtype specified by box |
There was a problem hiding this comment.
dtype is always object, the type contained is specified by box. let's also specify ndarray instead of just array
| def format_array_from_datetime(ndarray[int64_t] values, object tz=None, | ||
| object format=None, object na_rep=None): | ||
| def format_array_from_datetime( | ||
| ndarray[int64_t] values, |
There was a problem hiding this comment.
possibly can make const int64_t[:]?
There was a problem hiding this comment.
I think should hold off on these types of changes until we get warnings to error, unless there is a known upside
There was a problem hiding this comment.
its the usage encouraged by the cython folks, supposed to be slightly faster. very marginal
There was a problem hiding this comment.
@MomIsBestFriend i think just some minor comments, anything that is mentioned as a perf change should be done independetnl. pls call out anything non-trivial (e.g. formatting) that is changed. or just don't do anything that is non formatting / style in this PR
| @@ -481,8 +481,7 @@ cdef class Interval(IntervalMixin): | |||
|
|
|||
| @cython.wraparound(False) | |||
| @cython.boundscheck(False) | |||
| def intervals_to_interval_bounds(ndarray intervals, | |||
| bint validate_closed=True): | |||
| def intervals_to_interval_bounds(ndarray intervals, bint validate_closed=True): | |||
There was a problem hiding this comment.
right, but that is orthogonal to this PR
pandas/_libs/tslib.pyx
Outdated
| If datetime, convert to datetime.datetime | ||
| If date, convert to datetime.date | ||
| If time, convert to datetime.time | ||
| If Timestamp, convert to pandas.Timestamp |
There was a problem hiding this comment.
lgtm as a style fixup; @jbrockmendel has valid comments but I think should all be done separate
|
@MomIsBestFriend IIUC cython support for black is under development, psf/black#359. |
Thank you @simonjayhawkins, me and my OCD are thrilled, I subscribed to that thread 😆 |
|
Can you merge master? Otherwise I think this is good to go |
|
@MomIsBestFriend looking at the diff of the merge master, it looks like you've lost some of your clean-ups. |
@simonjayhawkins Correct, in 1cc4e89 I have reverted everything lost in the merge conflicts. |
|
All green. @jreback you requested changes, if you want to have another look, this should be ready. |
|
ok as long as purely cosmetic |
|
Thanks @MomIsBestFriend |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff