Skip to content

TST: Add tests for PyArray_IntpConverter#16454

Merged
seberg merged 2 commits intonumpy:masterfrom
eric-wieser:test-intp-converter
May 31, 2020
Merged

TST: Add tests for PyArray_IntpConverter#16454
seberg merged 2 commits intonumpy:masterfrom
eric-wieser:test-intp-converter

Conversation

@eric-wieser
Copy link
Copy Markdown
Member

Split from gh-15886.

@charris
Copy link
Copy Markdown
Member

charris commented May 31, 2020

numpy/core/src/multiarray/_multiarray_tests.c.src:2051:26: warning: initialization makes pointer from integer without a cast [-Wint-conversion]

Tests fail because of too many warnings.

Copy link
Copy Markdown
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful, thanks Eric, does this basically mean we can merge the other PR next/soon? Putting this in.

}

PyObject *tup = PyArray_IntTupleFromIntp(dims.len, dims.ptr);
PyDimMem_FREE(dims.ptr);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case for your (or anyone else's) info, we also have a npy_free_cache_dims_obj() which puts the dims back into a cache. But I like this here, since its public API and documented like this anway.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that, but this is compiled as a separate module so isn't available

self.conv(2**64)

def test_too_many_dims(self):
assert self.conv([1]*32) == (1,)*32
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed we actually expose np.MAXDIMS, no matter though, modifying the test is trivial, and I am not sure I like that we expose it...

@seberg seberg merged commit 7bb4697 into numpy:master May 31, 2020
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