-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Implement _is_utc in timezones #17419
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17419 +/- ##
==========================================
- Coverage 91.15% 91.13% -0.02%
==========================================
Files 163 163
Lines 49534 49534
==========================================
- Hits 45153 45144 -9
- Misses 4381 4390 +9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls show any asv diffs.
pandas/_libs/tslibs/timezones.pyx
Outdated
from dateutil.tz import gettz as _dateutil_gettz | ||
|
||
|
||
from pytz.tzinfo import BaseTzInfo as _pytz_BaseTzInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some extra imports here (I know you may use them later), but let's add them later then.
In order to get asv to run, I had to move timezones.pyx from _libs/tslibs/ to just _libs/.
As usual, I don't see anything meaningful here, since the one function being defined in timezones.pyx is verbatim identical to the one it is replacing. |
we'll did your code affect asof? that is a non trivial diff |
you can test multiple things via a regex in the bench timezone is also relevant here |
I don't see anything in
One function is implemented in timezones.pyx:
This replaces identical (up to a different alias for |
A search for tz|timezone in the benchmarks brought up the timeseries.py benchmarks above, 2 hits in binary_ops.py, and 1 hit in groupby.py. Are there others I'm missing?
Of these,
|
since this code touches virtually all time related code, pls run with |
OK, I just started this running. For my own edification: I was under the impression that since this PR is moving a function but not changing it that it would obviously not affect perf. Is there some theoretical reason that is wrong? Is this something cython-specific? |
whenever you touch cython in a non-trivial way, we need to assure perf is comparable. in this case you are moving lots of code it is very easy to drop things. just as you always tests, always run perf. |
This is running, but it looks like the "time" regex is catching everything since all the bench methods start with "time_". |
|
these benches look really unstable. not sure why. These benches should be very stable unless you have something odd going on. |
you can try setting processor afinity so it runs on a single (same) processor, I recall you have a 'big' machine? |
Affinity is worth a shot.
|
Tinkering around with setup.py on an unrelated branch I found that .pyx files like
|
* implemented fix for GH issue pandas-dev#16953 * added tests for fix of issue pandas-dev#16953 * changed comments for git issue to pandas style GH# * changed linelength in tests, so all lines are less than 80 characters * added whatsnew entry * swaped conversion and filtering of values, for plot to also work with object dtypes * refomated code, so len(line) < 80 * changed whatsnew with timedelta and datetime dtypes * added support for datetimetz and extended tests * added reason to pytest.mark.xfail
…s with pyarrow < 0.6.0 (pandas-dev#17447) closes pandas-dev#17438
…t side position for monotonic decreasing indexes (pandas-dev#17272)
not sure what you think is extraneous. these a represent c dependencies. sure some of needs a rebase |
New PR sounds good. This will be easier to just show than describe. |
rebase this and can merge next |
setup.py
Outdated
@@ -474,11 +475,12 @@ def pxd(name): | |||
'depends': (['pandas/_libs/src/klib/khash_python.h'] | |||
+ _pxi_dep['hashtable'])}, | |||
'_libs.tslib': {'pyxfile': '_libs/tslib', | |||
'pxdfiles': ['_libs/src/util'], | |||
'pxdfiles': ['_libs/src/util', '_libs/lib'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you add this back? ( I agree it looks like it should be here, but what was failing to indicate that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a screwup I made while rebasing. Will remove momentarily.
ok lgtm. ping on green. |
ping |
thanks. a tip for you. squashing commits makes rebasing much easier. generally commits should follow logical flow of code and be purposeful. |
This is the first of a bunch of PRs to take the place of #17274.
git diff upstream/master -u -- "*.py" | flake8 --diff