-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CLN: small ops optimizations #28036
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: small ops optimizations #28036
Changes from 2 commits
f27fb1e
7ebd943
64659a4
ea57dba
eb31258
cc118a6
08098ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -540,9 +540,6 @@ def _combine_frame(self, other, func, fill_value=None, level=None): | |
this, other = self.align(other, join="outer", level=level, copy=False) | ||
new_index, new_columns = this.index, this.columns | ||
|
||
if self.empty and other.empty: | ||
return self._constructor(index=new_index).__finalize__(self) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is covered by a test? (in general, I would rather leave sparse alone, this is deprecated anyway) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it is. I understand the impulse to leave sparse alone, but ATM some of it is a barrier to simplifying ops code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I was just saying on the other PR as well, there is also the option to just leave sparse as is, but still go forward with simplifying the ops code for non-sparse There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can also make it easier for you to focus only on the ops code for normal frame (without additional complexity of sharing with sparse) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I understand. core.ops functions call the DataFrame/SparseDataFrame methods, so anything I want to do to change the behavior of the non-sparse methods needs to be reflected in the sparse methods |
||
|
||
new_data = {} | ||
if fill_value is not None: | ||
# TODO: be a bit more intelligent here | ||
|
@@ -569,13 +566,13 @@ def _combine_frame(self, other, func, fill_value=None, level=None): | |
).__finalize__(self) | ||
|
||
def _combine_match_index(self, other, func, level=None): | ||
new_data = {} | ||
|
||
if level is not None: | ||
raise NotImplementedError("'level' argument is not supported") | ||
|
||
this, other = self.align(other, join="outer", axis=0, level=level, copy=False) | ||
|
||
new_data = {} | ||
for col, series in this.items(): | ||
new_data[col] = func(series.values, other.values) | ||
|
||
|
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.
Why do we want to remove the asserts here? If someone is that set on getting this optimization couldn't they just use the -OO flag?
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 put these assertions in when trying to understand the ops code a while back. having a comment to the same effect performs the same task for the reader.