Skip to content

Simplify algebra in Week and SemiMonth offsets #18278

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

Merged
merged 1 commit into from
Nov 15, 2017

Conversation

jbrockmendel
Copy link
Member

This PR is careful to only touch Week-base and SemiMonth-based offsets, i.e. has no overlap with #18263. It does, however, simplify the algebra such that the end result looks a lot like 18263.

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

@codecov
Copy link

codecov bot commented Nov 14, 2017

Codecov Report

Merging #18278 into master will decrease coverage by 0.02%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18278      +/-   ##
==========================================
- Coverage    91.4%   91.38%   -0.03%     
==========================================
  Files         164      164              
  Lines       49878    49852      -26     
==========================================
- Hits        45590    45555      -35     
- Misses       4288     4297       +9
Flag Coverage Δ
#multiple 89.18% <95.23%> (-0.01%) ⬇️
#single 39.43% <0%> (-0.04%) ⬇️
Impacted Files Coverage Δ
pandas/tseries/offsets.py 97.06% <95.23%> (-0.06%) ⬇️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.8% <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 69472f9...1b5dbc1. Read the comment docs.

@jreback jreback added Clean Frequency DateOffsets labels Nov 14, 2017
@jreback
Copy link
Contributor

jreback commented Nov 14, 2017

do we have sufficient tests for all of these cases?

@jbrockmendel
Copy link
Member Author

do we have sufficient tests for all of these cases?

As in #18263, I think so, yes.

@jbrockmendel
Copy link
Member Author

do we have sufficient tests for all of these cases?

Looks like between the three active PRs unifying the apply methods, there is one line in LastWeekOfMonth.apply that does not currently have coverage. I'll make sure to hit that in the next follow-up.

Side-note. I went to check this on codecov by clicking the badge on pandas GH page and the link just goes to the image file for the badge itself. Looks like all the badges do that. I doubt that's the intended behavior.

@jreback jreback added this to the 0.22.0 milestone Nov 15, 2017
@jreback
Copy link
Contributor

jreback commented Nov 15, 2017

Side-note. I went to check this on codecov by clicking the badge on pandas GH page and the link just goes to the image file for the badge itself. Looks like all the badges do that. I doubt that's the intended behavio

hmm thought that worked.

@jreback jreback merged commit 9aaf50a into pandas-dev:master Nov 15, 2017
@jreback
Copy link
Contributor

jreback commented Nov 15, 2017

thanks!

@jreback
Copy link
Contributor

jreback commented Nov 15, 2017

you can click on the codecov report in the pr to see things FYI

@jbrockmendel jbrockmendel deleted the tslibs-offsets-weekdays2 branch December 8, 2017 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants