-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Use Series.where rather than np.where in clip #3998
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
Conversation
can u add release notes mention and another test that has NaT in the series? |
Good catch on NaT. I'll add some tests. Just to confirm, what behavior do you want in that case? This commit is currently consistent with 11.0 behavior, which is itself consistent with the fact that NaT is ordered strictly lower than other timestamps.
At the expense of explicit null checking and some backwards incompatibility, we could instead make the behavior more nan-like. Thoughts? |
these should work with out u specifying value directly as far as NaT go they should be completely excluded u need to compute the mask first, then do the where using that mask (so that those values not in the mask are only ones included), then put the Nan's back in place this should work for datetimes and other dtypes I'll give u a reference to look at |
actually I think this easier that I said just make the cond = (s > 0) & notnull(s) I think will do the right thing |
I have it coded like this:
I'll finish up the PR tomorrow. |
ok that looks right throw in some test cases including some with nan (and different dtypes) I u can |
see if you can finish this up...want to get it itn |
Build failed in Python 3 for the case when the threshold is None. What behavior do you want in that case? Throw an exception? Return a copy of the series? Or just take it out of the test as a nonsensical condition? |
if the threshold is |
Python 3 fails test,
|
Yep. I don't have a Python 3 environment yet. Building one now... |
Is that really true, @jreback? It short-circuits at the element level? |
fyi you also might be interestd in a |
yes
|
@mtki you resolved the python3 issue? |
Builds are now passing. |
@mtkni needs a rebase (probably due to conflict in release.rst) |
Rebased. |
BUG: Use Series.where rather than np.where in clip
Great! |
Closes #3996