Skip to content

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

Merged
ahaldane merged 1 commit into
numpy:masterfrom
gerritholl:fix-structured-masked-array-str
Feb 22, 2016
Merged

ahaldane merged 1 commit into
numpy:masterfrom
gerritholl:fix-structured-masked-array-str

Conversation

@gerritholl

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Copy link
Copy Markdown
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

charris commented Feb 22, 2016

Copy link
Copy Markdown
Member

@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.
@gerritholl
gerritholl force-pushed the fix-structured-masked-array-str branch from 1841fe8 to 576b48f Compare February 22, 2016 16:41
@ahaldane

Copy link
Copy Markdown
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
BUG/TST: Fix #7259, do not "force scalar" for already scalar str/bytes
@ahaldane
ahaldane merged commit bec40c2 into numpy:master Feb 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants