implement tslibs.arraylike #21700
implement tslibs.arraylike #21700
Conversation
| @@ -37,6 +37,9 @@ from cython cimport Py_ssize_t | |||
| import pytz | |||
| UTC = pytz.utc | |||
|
|
|||
| from tslibs.arraylike import ( # noqa:F841 | |||
| format_array_from_datetime, array_to_datetime, array_with_unit_to_datetime, | |||
There was a problem hiding this comment.
isn’t the point to not import back here, rather import directly to where it’s needed
There was a problem hiding this comment.
Sure. For the first pass I like to keep the diff small, but now I'll update.
There was a problem hiding this comment.
Actually on second thought there are a lot of usages of these I'd rather not update. I think the thing to do is leave this where it is for now and put it in the tslibs namespace a few PRs from now when we finish transitioning tslib
Codecov Report
@@ Coverage Diff @@
## master #21700 +/- ##
=======================================
Coverage 91.9% 91.9%
=======================================
Files 154 154
Lines 49659 49659
=======================================
Hits 45640 45640
Misses 4019 4019
Continue to review full report at Codecov.
|
|
|
||
| cimport numpy as cnp | ||
| from numpy cimport int64_t, ndarray, float64_t | ||
| import numpy as np |
There was a problem hiding this comment.
so some of this should be put in conversion. or is there a reason you are not doing that? i would like to shy away from creating a new module unless it has a clear purpose. can you move what is logical first.
There was a problem hiding this comment.
so some of this should be put in conversion. or is there a reason you are not doing that?
Three broad reasons.
-
the dependency graph. Nothing else in
tslibsrelies on these functions, so I'd like to keep them away from the "low-level" code (recall beforetslibswas created there was just one giant file and it was a pain to tell what depended on what).1b) The original scope for
conversionwasconvert_to_tsobjectand the minimal set of functions that uses. There has been a little drift from there but its still recognizable. We should keep it that way. -
array_to_datetimedoes things slightly (and annoyingly) differently from the constructors inconversion, so I prefer to keep them separate until thearray_to_datetimebehavior is changed so they can share code. -
conversion is 1180 lines ATM which is near the high end of ideal IMO.
There was a problem hiding this comment.
ok, I guess my general comment is that I would like to logically group routines. not sure if all of this should be moved to a new single file (e.g. maybe they logicall belong in different modules). I haven't looked in detail.
all +1 for removing from tslibs.pyx. I think the hangup is that arraylike is not very descriptive (and conversions.pyx is). maybe a better name would help.
| @@ -0,0 +1,717 @@ | |||
| # -*- coding: utf-8 -*- | |||
|
|
|||
There was a problem hiding this comment.
if you end up creating this file, can you take the opportunity of the refactor to add a module docstring here to better document what / what type of functionality is included in this file?
But I agree with Jeff that from a quick view some of those functions seems to be about parsing / conversion?
There was a problem hiding this comment.
I'll add a docstring. See comment above about scope.
|
Part of this PR is superseded by #21721. The other part is in limbo. Closing to clear the queue. |
|
Woops, closed the wrong one. |
|
Closing as non-actionable for now |
git diff upstream/master -u -- "*.py" | flake8 --diff