diff reduction for 24024 #24543
diff reduction for 24024 #24543
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24543 +/- ##
==========================================
+ Coverage 92.32% 92.33% +<.01%
==========================================
Files 166 166
Lines 52454 52457 +3
==========================================
+ Hits 48430 48434 +4
+ Misses 4024 4023 -1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24543 +/- ##
==========================================
+ Coverage 92.32% 92.32% +<.01%
==========================================
Files 166 166
Lines 52454 52457 +3
==========================================
+ Hits 48430 48433 +3
Misses 4024 4024
Continue to review full report at Codecov.
|
gfyoung
added
Testing
There was a problem hiding this comment.
Truth be told, as someone who hasn't looked too closely at #24024, it's hard to understand what you mean by "correct" regarding these changes. By itself, it would have been tricky to understand the motivation.
That being said, as you are pulling this out of an existing PR, I'll trust that these will mesh with what you're ultimately aiming to do there.
pandas/core/indexes/period.py
Outdated
| return self._data.freq | ||
|
|
||
| @freq.setter | ||
| def freq(self, value): | ||
| value = Period._maybe_convert_freq(value) | ||
| # Note: When this deprecation is enforced, PeriodIndex.freq can |
| @@ -3245,7 +3245,8 @@ def test_setitem(self): | |||
| b1 = df._data.blocks[1] | |||
| b2 = df._data.blocks[2] | |||
| assert b1.values.equals(b2.values) | |||
| assert id(b1.values.values.base) != id(b2.values.values.base) | |||
| if b1.values.values.base is not None: | |||
There was a problem hiding this comment.
add a comment here on what you are checking
There was a problem hiding this comment.
should be covered by the comment 2 lines up
pandas/core/arrays/datetimelike.py
Outdated
| if not is_period_dtype(self): | ||
| return type(self)(res_values, freq='infer') | ||
| return self._from_sequence(res_values) | ||
| kwargs['freq'] = 'infer' |
There was a problem hiding this comment.
I prefer the original way of calling this, e.g.
self._from_sequence(res_values, freq='infer')
|
Again, lodging a request that we stop breaking pieces off. |
Aside from a diff-reduction PR made on your branch moments before this request came in: done. |
|
@TomAugspurger close this an proceed with #24024 ? ok by me |
|
This is pretty benign and cuts 10% off of 24024 |
|
plus a pretty test |
|
I haven't looked at this yet, so I can't choose in this specific case. Just
stating a general preference.
…On Wed, Jan 2, 2019 at 7:53 AM Jeff Reback ***@***.***> wrote:
@TomAugspurger <https://github.com/TomAugspurger> close this an proceed
with #24024 <#24024> ? ok by me
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24543 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIj68Orkq-mMoKTVAcPYyikT3Heuqks5u_LnBgaJpZM4ZmMGY>
.
|
|
ping |
|
ok, over to you @TomAugspurger for #24024 |
|
@jreback why did you merge this? I understand it as @TomAugspurger explicitly asked for stopping breaking off pieces? |
docstrings, comments, edits that are correct both before and after #24024
the only substantive change is adding a test specifically for constructing a DatetimeArray from a PandasArray.