Skip to content
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

Move fields functions out of the way #17770

Merged
merged 7 commits into from
Oct 4, 2017

Conversation

jbrockmendel
Copy link
Member

Notes:

  • The only usage in tslib of get_date_name_field is for Timestamp.weekday and can be simplified with a more direct call.

  • Several of the functions from tslibs.fields imported into tslib are unused. Updating the external imports to use the new locations is for a follow-up.

  • There is some flake8 cleanup to be done in the the new tslibs.fields, but that is being saved for a follow-up. For now this is just cut/paste.

  • get_start_end_field will be largely unnecessary if/when the suggestion to deprecate Timestamp.freq in DEPR: remove freq attribute of Timestamp? #15146 is adopted.

  • closes #xxxx

  • tests added / passed

  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

  • whatsnew entry

@pep8speaks
Copy link

pep8speaks commented Oct 4, 2017

Hello @jbrockmendel! Thanks for updating the PR.

  • In the file setup.py, following are the PEP8 issues :

Line 492:80: E501 line too long (82 > 79 characters)
Line 493:80: E501 line too long (92 > 79 characters)

Comment last updated on October 04, 2017 at 14:36 Hours UTC

@codecov
Copy link

codecov bot commented Oct 4, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17770      +/-   ##
==========================================
- Coverage   91.26%   91.24%   -0.02%     
==========================================
  Files         163      163              
  Lines       49872    49872              
==========================================
- Hits        45514    45505       -9     
- Misses       4358     4367       +9
Flag Coverage Δ
#multiple 89.04% <ø> (ø) ⬆️
#single 40.24% <ø> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.73% <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 81694dc...7b12b5a. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 4, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17770      +/-   ##
==========================================
- Coverage   91.26%   91.24%   -0.02%     
==========================================
  Files         163      163              
  Lines       49872    49872              
==========================================
- Hits        45514    45505       -9     
- Misses       4358     4367       +9
Flag Coverage Δ
#multiple 89.04% <ø> (ø) ⬆️
#single 40.24% <ø> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.73% <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 81694dc...7b12b5a. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 4, 2017

Codecov Report

Merging #17770 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17770      +/-   ##
==========================================
- Coverage   91.26%   91.22%   -0.04%     
==========================================
  Files         163      163              
  Lines       49872    49917      +45     
==========================================
+ Hits        45514    45536      +22     
- Misses       4358     4381      +23
Flag Coverage Δ
#multiple 89.02% <100%> (-0.02%) ⬇️
#single 40.24% <50%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/period.py 92.78% <100%> (+0.01%) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/indexing.py 93% <0%> (-0.98%) ⬇️
pandas/core/frame.py 97.73% <0%> (-0.1%) ⬇️
pandas/core/generic.py 92.1% <0%> (-0.03%) ⬇️
pandas/core/common.py 91.42% <0%> (ø) ⬆️

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 81694dc...0e9e1e5. Read the comment docs.

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.

lgtm. some comments. ping on green.

from numpy cimport ndarray, int64_t, int32_t, int8_t
np.import_array()

cdef int64_t NPY_NAT = np.datetime64('NaT').astype(np.int64)
Copy link
Contributor

Choose a reason for hiding this comment

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

use util.get_nat(), don't change the incantation

Copy link
Member Author

Choose a reason for hiding this comment

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

Had hoped to avoid dependency; changing.

return days_per_month_table[is_leapyear(dts.year)][dts.month -1]


cpdef _isleapyear_arr(ndarray years):
Copy link
Contributor

Choose a reason for hiding this comment

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

can de-privatize

cdef:
ndarray[int8_t] out

# to make NaT result as False
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ok to remove this comment (instead add a doc-string)

@jreback jreback added Datetime Datetime data dtype Clean labels Oct 4, 2017
@jreback
Copy link
Contributor

jreback commented Oct 4, 2017

get_start_end_field will be largely unnecessary if/when the suggestion to deprecate Timestamp.freq in #15146 is adopted.

I think this would be a difficult change FYI

@jreback jreback added this to the 0.21.0 milestone Oct 4, 2017
@jreback jreback merged commit a26afca into pandas-dev:master Oct 4, 2017
@jreback
Copy link
Contributor

jreback commented Oct 4, 2017

thanks!

ghost pushed a commit to reef-technologies/pandas that referenced this pull request Oct 16, 2017
@jbrockmendel jbrockmendel deleted the tslibs-fields branch October 30, 2017 16:22
alanbato pushed a commit to alanbato/pandas that referenced this pull request Nov 10, 2017
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Datetime Datetime data dtype
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants