-
Notifications
You must be signed in to change notification settings - Fork 357
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
Implement DataFrame.insert #1983
Conversation
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'll take a more closer look after passing the tests :)
Codecov Report
@@ Coverage Diff @@
## master #1983 +/- ##
==========================================
- Coverage 94.59% 94.59% -0.01%
==========================================
Files 50 50
Lines 11194 11210 +16
==========================================
+ Hits 10589 10604 +15
- Misses 605 606 +1
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.
I found some bugs:
>>> kdf = ks.DataFrame([1, 2, 3], index=[10,20,30])
>>> kdf.insert(1, 'z', ks.Series([8, 9, 10]))
>>> kdf
0 z
0 NaN 8.0
1 NaN 9.0
10 1.0 NaN
2 NaN 10.0
30 3.0 NaN
20 2.0 NaN
>>> pdf = pd.DataFrame([1, 2, 3], index=[10,20,30])
>>> pdf.insert(1, 'z', pd.Series([8, 9, 10]))
>>> pdf
0 z
10 1 NaN
20 2 NaN
30 3 NaN
or
>>> kdf = ks.DataFrame({('x', 'a'): [1, 2, 3]})
>>> kdf.insert(1, 'z', ks.Series([8, 9, 10]))
Traceback (most recent call last):
...
AssertionError: [('x', 'a'), ('z',)]
>>> pdf = pd.DataFrame({('x', 'a'): [1, 2, 3]})
>>> pdf.insert(1, 'z', pd.Series([8, 9, 10]))
>>> pdf
x z
a
0 1 8
1 2 9
2 3 10
08f60ea
to
8828b29
Compare
databricks/koalas/frame.py
Outdated
kdf = kdf[columns] | ||
self._update_internal_frame(kdf._internal) | ||
|
||
# TODO: add frep and axis parqameter |
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.
Shall we revert this change?
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.
May I ask why?
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 was thinking kdf
and self
should share the same anchor.
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 mean this line # TODO: add frep and axis parqameter
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.
btw
I was thinking kdf and self should share the same anchor.
kdf
could have a different anchor after assigning the value
if the value
is not the same anchor.
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 see, may I get some help understand requires_same_anchor
parameter of _update_internal_frame
?
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.
If it's False
, the existing linked Series
will see the updates even when the anchor changes; otherwise the link will be disconnected.
See #1592 for more detail.
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.
Thanks for the reference!
No matter what requires_same_anchor
is, the test case doesn't seem to pass https://github.com/databricks/koalas/pull/1983/files#diff-028bf26a42786beb47e6707fe34867dc720d3279ae4c942226abc3eb40f26eeaR104.
Would you give me a clue?
e6269b0
to
eef97a6
Compare
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.
Thanks! merging. |
@ueshin Thank you! |
ref #1929
Insert column into DataFrame at a specified location.