Follow-Up: Unify apply and onOffset implementations #18329
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #18329 +/- ##
==========================================
- Coverage 91.38% 91.36% -0.02%
==========================================
Files 164 164
Lines 49780 49786 +6
==========================================
- Hits 45491 45488 -3
- Misses 4289 4298 +9
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #18329 +/- ##
==========================================
- Coverage 91.38% 91.34% -0.05%
==========================================
Files 164 164
Lines 49790 49796 +6
==========================================
- Hits 45501 45486 -15
- Misses 4289 4310 +21
Continue to review full report at Codecov.
|
jreback
requested changes
Nov 17, 2017
pandas/_libs/tslibs/offsets.pyx
Outdated
| @@ -394,6 +394,10 @@ class _BaseOffset(object): | |||
| out = '<%s' % n_str + className + plural + self._repr_attrs() + '>' | |||
| return out | |||
|
|
|||
| def _get_offset_day(self, datetime other): | |||
| # subclass must implement `_day_opt` | |||
There was a problem hiding this comment.
so in the Base class add a NotImplementedError for this
jreback
requested changes
Nov 17, 2017
pandas/_libs/tslibs/offsets.pyx
Outdated
| else: | ||
| # Note: unlike `shift_month`, get_day_of_month does not |
There was a problem hiding this comment.
can you add a test with a sub-class of BaseOffset (e.g. a dummy class) that checks this
|
Appveyor error looks like floating point error in an unrelated test. Will push dummy commit in a bit. |
|
can you rebase |
…libs-offsets-followup
|
looks fine. can you run the offset asvs & report
is? |
|
Pretty tame by local standards: |
|
One more: |
jreback
approved these changes
Nov 19, 2017
|
thanks! |
jreback
reviewed
Nov 19, 2017
| @@ -357,6 +357,7 @@ class _BaseOffset(object): | |||
| _typ = "dateoffset" | |||
| _normalize_cache = True | |||
There was a problem hiding this comment.
in a followup (TODO), can you add a description of these attributes in the doc-string of this class. (e.g. what the options are, what they do, etc).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This follows up on #18280, #18278, and #18263.
get_day_of_monthis extended to handlebusiness_startandbusiness_endin addition tostartandend._day_optattributes are added to YearFoo, QuarterFoo, and MonthFoo offsets. After some more algebraic simplification (done in a separate PR), their apply and onOffset methods will become identical and can be implemented in the base Year, Quarter, and Month classes.I expect a small performance penalty from calling
self._get_offset_dayin cases where could hard-code1, but the benefits of a unified+simplified implementation outweigh that.