-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
BUG/TST: Fix #7259, do not "force scalar" for already scalar str/bytes #7260
BUG/TST: Fix #7259, do not "force scalar" for already scalar str/bytes #7260
Conversation
ba2bb65
to
1841fe8
Compare
Seems good, although a deeper fix would be to make scalar strings allow indexing with an empty tuple. @gerritholl , are you interested in looking at a deeper fix, or would you rather commit this PR now and open a new issue to fix it properly later? I'm fine either way. In the latter case we should open an issue linking to this one, and add a comment in the code "(this can be removed once #XXXX is fixed)". |
Here are some beginning investigations about how the deeper issue (tuple indexing of strings) can be fixed. I can't totally see the solution, and it's harder, but I think it involves the following in A. Define a function
B. Define (analagous to
C. in
The part I don't see yet is what exactly to put in to |
I agree that this might be a better fix. Unfortunately, I have very little experience writing C and no experience at all writing C/Python, so I don't consider myself the right person to implement the change such as you propose. |
All right, I'd like to merge then, after a possible tweak. Could we change it to
instead of the try/catch, because it maybe makes clear we are accounting for the ndarray vs scalar cases, if you agree? |
@gerritholl Could you make the suggested fix? |
…/bytes Fix for numpy#7259. Since commit f8f753b, fill_value was forced scalar by indexing with [()]. This works for most numpy scalars, but not for U (str) and S (bytes). This commit falls back to the pre-f8f753b version when forcing to scalar fails for fill_value of dtypes with kind U and S. Also add a test to see that this works in practice.
1841fe8
to
576b48f
Compare
LGTM, merging. Thanks @gerritholl. (By the way I think this is the correct fix even if #7267 is solved, since #7267 cannot fix the case of object arrays) |
BUG/TST: Fix #7259, do not "force scalar" for already scalar str/bytes
Fix for #7259. Since commit f8f753b, fill_value was forced scalar by
indexing with [()]. This works for most numpy scalars, but not for U
(str) and S (bytes). This commit falls back to the pre-f8f753b version
when forcing to scalar fails for fill_value of dtypes with kind U and S.
Also add a test to see that this works in practice.