-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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
CLN: standardize different freq message #24283
Conversation
Hello @jbrockmendel! Thanks for updating the PR.
Comment last updated on December 14, 2018 at 22:23 Hours UTC |
pandas/core/arrays/period.py
Outdated
freqstr=self.freqstr)) | ||
if np.ndim(other) > 0: | ||
# FIXME: What should this be rendered as? | ||
other_freq = "FIXME" |
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.
Does "None" make sense here?
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.
Either that or something resembling a least common denominator. None sounds easier.
+1 |
I can take care of that if you like.
…On Fri, Dec 14, 2018 at 2:17 PM jbrockmendel ***@***.***> wrote:
I suppose that would be a relatively easy fix to split off from #24024
<#24024>, and it'd mean fewer
changes here as well.
+1
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#24283 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIqZ0n94rK0FFty2oDiHX_JyK4Swdks5u5AdNgaJpZM4ZULGn>
.
|
Codecov Report
@@ Coverage Diff @@
## master #24283 +/- ##
==========================================
+ Coverage 92.22% 92.22% +<.01%
==========================================
Files 162 162
Lines 51828 51836 +8
==========================================
+ Hits 47799 47807 +8
Misses 4029 4029
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24283 +/- ##
==========================================
+ Coverage 92.22% 92.22% +<.01%
==========================================
Files 162 162
Lines 51824 51828 +4
==========================================
+ Hits 47795 47799 +4
Misses 4029 4029
Continue to review full report at Codecov.
|
lgtm. @TomAugspurger if ok for you to merge ping (or merge) |
@@ -372,15 +372,13 @@ def __setitem__( | |||
value = period_array(value) | |||
|
|||
if self.freqstr != value.freqstr: |
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.
PeriodArray.__setitem__
is being removed, so I was hoping to not change these.
* standardize different freq message * implement raise_on_incompatible * fixup missing import * freqstr cases * update tested messages
* standardize different freq message * implement raise_on_incompatible * fixup missing import * freqstr cases * update tested messages
In #24282 some of the PeriodArray/PeriodIndex methods couldn't accept the decorator because of mismatches between _DIFFERENT_FREQ and DIFFERENT_FREQ_INDEX. This unifies the two.
The remaining case to figure out is what to render for
other_freq
when dealing with an ndarray. Right now the placeholder is "FIXME".