8000 BUG/TST: Fix #7259, do not "force scalar" for already scalar str/bytes by gerritholl · Pull Request #7260 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG/TST: Fix #7259, do not "force scalar" for already scalar str/bytes #7260

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 22, 2016
Merged

BUG/TST: Fix #7259, do not "force scalar" for already scalar str/bytes #7260

merged 1 commit into from
Feb 22, 2016

Conversation

gerritholl
Copy link
Contributor

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.

@gerritholl gerritholl force-pushed the fix-structured-masked-array-str branch 2 times, most recently from ba2bb65 to 1841fe8 Compare February 16, 2016 16:16
@ahaldane
Copy link
Member

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)".

@ahaldane
Copy link
Member

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 scalartypes.c.src:

A. Define a function stringtype_subscript (analagous to gen_arrtype_subscript), see below

static PyObject *
stringtype_subscript(PyObject *self, PyObject *key) {

B. Define (analagous to gentype_as_mapping)

static PyMappingMethods stringtype_as_mapping = {
    NULL,
    (binaryfunc)stringtype_subscript,
    NULL
};

C. in initialize_numeric_types do

PyStringArrType_Type.as_mapping = &stringtype_as_mapping;
PyUnicodeArrType_Type.as_mapping = &stringtype_as_mapping;

The part I don't see yet is what exactly to put in to stringtype_subscript. Probably it should check for an empty tuple and return self if so, otherwise call "super" on the string/unicode getitem/setitem. But I'm not sure how the super call works. I also haven't figured out how/where the numpy scalar-type inheritance is set up.

@gerritholl
Copy link
Contributor Author

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.

@ahaldane ahaldane mentioned this pull request Feb 17, 2016
Open
@ahaldane
Copy link
Member

All right, I'd like to merge then, after a possible tweak.

Could we change it to

if isinstance(self._fill_value, ndarray):
    return self._fill_value[()]
return self._fill_value

instead of the try/catch, because it maybe makes clear we are accounting for the ndarray vs scalar cases, if you agree?

@charris
Copy link
Member
charris commented Feb 22, 2016

@gerritholl Could you make the suggested fix?

@gerritholl
BUG/TST: Fix numpy#7259, do not "force scalar" for already scalar str…
576b48f 
…/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.
@gerritholl gerritholl force-pushed the fix-structured-masked-array-str branch from 1841fe8 to 576b48f Compare February 22, 2016 16:41
@ahaldane
Copy link
Member

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)

ahaldane added a commit that referenced this pull request Feb 22, 2016
@ahaldane
Merge pull request #7260 from gerritholl/fix-structured-masked-array-str
bec40c2 
BUG/TST: Fix #7259, do not "force scalar" for already scalar str/bytes
@ahaldane ahaldane merged commit bec40c2 into numpy:master Feb 22, 2016
@ahaldane ahaldane mentioned this pull request Apr 29, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
00 - Bug component: numpy.ma masked arrays
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gerritholl @ahaldane @charris
0