Skip to content

MAINT: fix use of cache_dim #13724

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
Jun 7, 2019
Merged

MAINT: fix use of cache_dim #13724

merged 1 commit into from
Jun 7, 2019

Conversation

mattip
Copy link
Member

@mattip mattip commented Jun 6, 2019

in #13691 I noticed the strange cleanup code used when unable to allocate the data memory. Setting fa->nd = 0 was part of the original code, and at some point the npy_free_cache_dim_array(self); line was added. But

  • setting nd = 0means the call to npy_free_cache_dim_array(self); could free the wrong cache chunk, and
  • npy_free_cache_dim_array(self); is called anyway when deallocating self
    so I think none of this is needed here.

It is hard to write tests for this, we need to fail to allocate data memory when unpickling.

Additionally, npy_alloc_cache_dim(3 * nd) is wrong, it should always be 2 * nd.

@mattip mattip mentioned this pull request Jun 6, 2019
Merged
mhvk
mhvk reviewed Jun 6, 2019
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

This looks good, with only a comment about perhaps trying to make the code a bit clearer.

@eric-wieser
Copy link
Member

cc @juliantaylor in case you thought about this in gh-8920

@eric-wieser
Copy link
Member

Ah, here's why it was 3:

numpy/numpy/core/src/multiarray/getset.c

Lines 67 to 77 in 3e32ef2

((PyArrayObject_fieldaccess *)self)->dimensions = PyDimMem_NEW(3*nd);
if (PyArray_DIMS(self) == NULL) {
Py_DECREF(ret);
PyErr_SetString(PyExc_MemoryError,"");
return -1;
}
((PyArrayObject_fieldaccess *)self)->strides = PyArray_DIMS(self) + nd;
((PyArrayObject_fieldaccess *)self)->maskna_strides = PyArray_DIMS(self) + 2*nd;
memcpy(PyArray_DIMS(self), PyArray_DIMS(ret), nd*sizeof(intp));
memcpy(PyArray_STRIDES(self), PyArray_STRIDES(ret), nd*sizeof(intp));
memcpy(PyArray_MASKNA_STRIDES(self), PyArray_MASKNA_STRIDES(ret), nd*sizeof(intp));

@eric-wieser
Copy link
Member

Would be nice to adjust the commit message to explain that 3 is leftover from when PyArray_MASKNA_STRIDES was around

@mattip mattip merged commit edd334c into numpy:master Jun 7, 2019
@mattip
Copy link
Member Author

mattip commented Jun 7, 2019

I put the PyArray_MASKNA_STRIDES remark in the merge comment

@mattip mattip deleted the error-cleanup branch August 8, 2019 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhvk mhvk mhvk left review comments

@eric-wieser eric-wieser eric-wieser approved these changes

Assignees
No one assigned
Labels
03 - Maintenance component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mattip @eric-wieser @mhvk