-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
REF: clear out a bunch of algos, de-duplicate a bunch of core.missing #24652
Conversation
""" | ||
Cast values to a dtype that algos.pad and algos.backfill can handle. | ||
""" | ||
# TODO: for int-dtypes we make a copy, but for everything else this |
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.
no idea why this would be. Would really try to be consistent. I think copy is more sematically correct for these types of routines, as we can't always guaranteee inplace behavior (meaning we may have to copy sometimes). So this may have some uncessary copying going on, but willing to trade that for better semantics.
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.
Yah, this came up when testing fillna from #24024. In at least a few of the places where this is used, we make a copy before calling, so we can trim some overhead by being a bit more careful.
I'm going to try to handle this more systematically at the same time I take a look at #24537. Would like to keep the comment there for the time being; I think this was just not noticed previously.
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.
absolutely. I am ok with inplace too. Its just then the guarantee to the caller will have to be stronger. We probably don't have enough tests in place to make a strong statement either way, but go for it. This looks like a nice re-factor, so could address the inplace/copy question later (as long as we are pretty sure its not actualy mutating things in the caller, unless its specifically allowed).
Codecov Report
@@ Coverage Diff @@
## master #24652 +/- ##
==========================================
+ Coverage 92.37% 92.37% +<.01%
==========================================
Files 166 166
Lines 52384 52317 -67
==========================================
- Hits 48390 48330 -60
+ Misses 3994 3987 -7
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #24652 +/- ##
==========================================
+ Coverage 92.37% 92.37% +<.01%
==========================================
Files 166 166
Lines 52384 52317 -67
==========================================
- Hits 48390 48330 -60
+ Misses 3994 3987 -7
Continue to review full report at Codecov.
|
@jbrockmendel happy to merge |
everyone happy here? |
lgtm |
git diff upstream/master -u -- "*.py" | flake8 --diff