Skip to content

Follow-up to #17419 #17497

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 12, 2017
Merged

Follow-up to #17419 #17497

merged 3 commits into from
Sep 12, 2017

Conversation

jbrockmendel
Copy link
Member

This moves a few other 3ish line functions from tslib to tslibs.timezones.

One non-trivial function _get_zone is cut/pasted over.

Replaces imports in inference.pyx and period.pyx with imports from tslibs.timezones. In the inference, case, replaces the import of the python version get_timezone with the cython version _get_zone

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@gfyoung gfyoung added Clean Internals Related to non-user accessible pandas implementation labels Sep 11, 2017
@codecov
Copy link

codecov bot commented Sep 12, 2017

Codecov Report

Merging #17497 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17497      +/-   ##
==========================================
- Coverage   91.15%   91.13%   -0.02%     
==========================================
  Files         163      163              
  Lines       49534    49534              
==========================================
- Hits        45153    45144       -9     
- Misses       4381     4390       +9
Flag Coverage Δ
#multiple 88.92% <ø> (ø) ⬆️
#single 40.22% <ø> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.72% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46856c3...6958c68. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 12, 2017

Codecov Report

Merging #17497 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17497      +/-   ##
==========================================
- Coverage   91.15%   91.13%   -0.02%     
==========================================
  Files         163      163              
  Lines       49534    49534              
==========================================
- Hits        45153    45144       -9     
- Misses       4381     4390       +9
Flag Coverage Δ
#multiple 88.92% <ø> (ø) ⬆️
#single 40.22% <ø> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.72% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46856c3...6958c68. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Sep 12, 2017

Codecov Report

Merging #17497 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17497      +/-   ##
==========================================
- Coverage   91.15%   91.13%   -0.02%     
==========================================
  Files         163      163              
  Lines       49534    49534              
==========================================
- Hits        45153    45144       -9     
- Misses       4381     4390       +9
Flag Coverage Δ
#multiple 88.92% <ø> (ø) ⬆️
#single 40.22% <ø> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.72% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46856c3...6958c68. Read the comment docs.

jreback
jreback reviewed Sep 12, 2017
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good
can u run asv (for relevant ones) and report if any issues

@jbrockmendel
Copy link
Member Author

Looks pretty benign:

asv continuous -E virtualenv -f 1.1 master HEAD -b tz -b timezone -b tzone -b utc -b date
       before           after         ratio
     [46856c39]       [6958c680]
+           290ms            407ms     1.40  gil.nogil_read_csv.time_read_csv_datetime
-           16.3s            14.2s     0.87  gil.nogil_datetime_fields.time_datetime_to_period
-           48.3s            41.3s     0.86  gil.nogil_datetime_fields.time_period_to_datetime
-           37.6s            31.6s     0.84  gil.nogil_datetime_fields.time_datetime_field_normalize
-           20.2s            16.3s     0.81  gil.nogil_datetime_fields.time_datetime_field_year
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.

@jreback jreback added this to the 0.21.0 milestone Sep 12, 2017
@jreback jreback merged commit 34cc2e8 into pandas-dev:master Sep 12, 2017
@jreback
Copy link
Contributor

jreback commented Sep 12, 2017

thanks.

before moving on, can you rename all _* functions in the new modules (tslis/*) to remove the _. These modules themselves are private, not need to have a double layer of indirection.

e.g. _get_zone -> get_zone (actually rename this back to get_timezone), not sure why changed originally.

@jbrockmendel
Copy link
Member Author

Will do.

e.g. _get_zone -> get_zone (actually rename this back to get_timezone), not sure why changed originally.

Maybe this predates the cpdef syntax? get_timezone is currently a regular def function that just passes through to the cdef'd _get_zone.

Speaking of privateness, I've been leaving behind in tslib unused imports from tslibs.foo import _bar # noqa so as to leave the namespace unchanged for backward compat. Are these unnecessary?

@jbrockmendel jbrockmendel mentioned this pull request Sep 12, 2017
Merged
4 tasks
@jbrockmendel jbrockmendel deleted the tslibs-timezones3 branch October 30, 2017 16:24
alanbato pushed a commit to alanbato/pandas that referenced this pull request Nov 10, 2017
@jbrockmendel @alanbato
Follow-up to pandas-dev#17419 (pandas-dev#17497)
6d17358
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017
@jbrockmendel @No-Stream
Follow-up to pandas-dev#17419 (pandas-dev#17497)
17a616e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback left review comments

Assignees
No one assigned
Labels
Clean Internals Related to non-user accessible pandas implementation
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jbrockmendel @jreback @gfyoung