-
Notifications
You must be signed in to change notification settings - Fork 358
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
Fix Dataframe.melt function & Add doctest case for melt function #987
Conversation
nice finding |
Codecov Report
@@ Coverage Diff @@
## master #987 +/- ##
==========================================
+ Coverage 94.83% 94.96% +0.13%
==========================================
Files 34 34
Lines 6640 6713 +73
==========================================
+ Hits 6297 6375 +78
+ Misses 343 338 -5
Continue to review full report at Codecov.
|
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.
@fwani Good catch!
But I'm afraid the fix is not a good direction. Please see the comment below.
Thanks!
…e not present in the Dataframe
@ueshin |
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.
Otherwise, LGTM.
databricks/koalas/frame.py
Outdated
raise KeyError("None of [{}] are in the [columns]" | ||
.format(pd.MultiIndex.from_tuples(non_existence_col))) |
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.
The error message is strange?
KeyError: "None of [MultiIndex(levels=[['X'], ['Y']],\n codes=[[0], [0]])] are in the [columns]"
How about:
- raise KeyError("None of [{}] are in the [columns]"
- .format(pd.MultiIndex.from_tuples(non_existence_col)))
+ raise KeyError("None of {} are in the {}"
+ .format(non_existence_col, column_index))
then
KeyError: "None of [('X', 'Y')] are in the [('X', 'A'), ('X', 'B'), ('Y', 'C')]"
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.
I agree with you.
I'll change error message the case of id_vars, value_vars.
databricks/koalas/frame.py
Outdated
raise KeyError("None of [{}] are in the [columns]" | ||
.format(pd.MultiIndex.from_tuples(non_existence_col))) |
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.
ditto.
Softagram Impact Report for pull/987 (head commit: 551b3ea)⭐ Change Overview
📄 Full report
Impact Report explained. Give feedback on this report to [email protected] |
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.
LGTM.
Thanks! merging. |
There is no test that tested with
str, tuple type
ofvalue_vars
parameter at Dataframe.melt function.I update doctest case.
And, I thought, There is a bug like the image below.
I changed some codes.