DEPR: Error with ambiguous groupby strings #22415
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22415 +/- ##
==========================================
- Coverage 92.05% 92.04% -0.01%
==========================================
Files 169 169
Lines 50713 50708 -5
==========================================
- Hits 46683 46675 -8
- Misses 4030 4033 +3
Continue to review full report at Codecov.
|
| # descending order | ||
| expected = df.sort_index(level='idx', ascending=False) | ||
| assert_frame_equal(result, expected) | ||
|
|
There was a problem hiding this comment.
are these sufficiently covered as testing that they raise?
There was a problem hiding this comment.
Raise, but we already have coverage.
| [pd.Grouper(key='inner')], | ||
| [pd.Grouper(level='inner')] | ||
| ), | ||
| (['B', 'inner'], # Column and index |
There was a problem hiding this comment.
maybe just change to have these check for raising? or too duplicative?
There was a problem hiding this comment.
Too duplicative --> it raises.
| columns=['YEAR', 'MONTH'], | ||
| values=['DAYS', 'SALARY'], | ||
| aggfunc={'DAYS': 'mean', 'SALARY': 'sum'}, | ||
| margins=True) |
There was a problem hiding this comment.
Yes, but we already have coverage for raising.
| # Construct right_df with an index level named 'outer' | ||
| right_df = df2.set_index('outer') | ||
|
|
||
| # Construct expected result. |
There was a problem hiding this comment.
Yes, but we already have coverage for raising.
|
@jreback : Any further thoughts on this PR? I answered all of your questions. |
|
Running dask's test suite on this branch quick. I suspect this will break some things (because dask has unhandled warnings). |
|
@TomAugspurger if might make sense to expand the test_dask test that we have (which basically just tests imports). |
|
Perhaps, but this would have been a minor edge case.
Things seem good on dask's end, +1
…On Wed, Aug 22, 2018 at 7:28 AM Jeff Reback ***@***.***> wrote:
@TomAugspurger <https://github.com/TomAugspurger> if might make sense to
expand the test_dask test that we have (which basically just tests imports).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22415 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIjuPLxAW0QN2GT3U-Ry3aM4u3c7eks5uTU5ogaJpZM4WC2qa>
.
|
|
thanks @gfyoung |
| issued and the column takes precedence. This will result in an ambiguity error | ||
| in a future version. | ||
| If a string matches both a column name and an index level name, a | ||
| ``ValueError`` will be raised. |
There was a problem hiding this comment.
This should no longer be in a versionadded directive?
There was a problem hiding this comment.
Do we drop ..versionadded directives from docstrings?
There was a problem hiding this comment.
This is not a docstring, but yes, we do drop them from time to time (but not that often).
However, in this case, what is stated is now not correct anymore. There was no error added in 0.20. So maybe we can also change the 0.20 to 0.24 ?
There was a problem hiding this comment.
I might just refer to drop it, since it really isn't an add either. Happy to debate semantics 🙂
Follow-up to pandas-devgh-22415.
Title is self-explanatory.
xref #14432.