Skip to content
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

MAINT: Clarify the error message for resize failure #12121

Merged
merged 2 commits into from
Oct 19, 2018

Conversation

kngwyu
Copy link
Contributor

@kngwyu kngwyu commented Oct 9, 2018

Curerntly

>>> arr = np.array([1, 2, 3])
>>> arr_ref = arr
>>> arr.resize(100)

causes this error.

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: cannot resize an array that references or is referenced
by another array in this way.  Use the resize function

But, I feel it odd to tell users to use resize, when resize fails.

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

The wording is "use the resize function", not "use the resize method". Perhaps it would be clearer to say use the np.resize function

reshape is not a substitute for resize.

@kngwyu kngwyu changed the title BUG: Fix error message for resize failure ENH: Clarify the error message for resize failure Oct 9, 2018
@kngwyu
Copy link
Contributor Author

kngwyu commented Oct 9, 2018

Thanks, I changed to just propose np.resize or refcheck=False.

@charris charris changed the title ENH: Clarify the error message for resize failure MAINT: Clarify the error message for resize failure Oct 10, 2018
@@ -108,7 +108,7 @@ PyArray_Resize(PyArrayObject *self, PyArray_Dims *newshape, int refcheck,
PyErr_SetString(PyExc_ValueError,
"cannot resize an array that "
"references or is referenced\n"
"by another array in this way. Use the resize function");
"by another array in this way. Use the np.resize function or refcheck=False");
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct - passing refcheck = False doesn't help if PyArray_BASE(arr) is set.

Either:

  • remove the remark about refcheck
  • Split this if into two ifs, and only mention refcheck in the one where refcnt > 2 is true.

@kngwyu
Copy link
Contributor Author

kngwyu commented Oct 14, 2018

Thanks, fixed by the later way.

PyErr_SetString(PyExc_ValueError,
"cannot resize an array that "
"references or is referenced\n"
"by another array in this way. Use the np.resize function.");
return NULL;
}
Copy link
Member

Choose a reason for hiding this comment

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

As a follow up, you could move this check to be done before the other two. As you have it, there are cases where the user sees "A is not allowed, try B or C", then tries B and gets "B is not allowed, try C". If you move this check first, then the user will see "A is not allowed, try C", and fix it in one step.

@kngwyu
Copy link
Contributor Author

kngwyu commented Oct 14, 2018

Thanks, applied your advice.

@eric-wieser
Copy link
Member

Would be great to add a test for that code path that codecov complains about. Creating a weakref to the array then trying to resize it should do the trick.

@kngwyu
Copy link
Contributor Author

kngwyu commented Oct 18, 2018

Thanks. Please let me know if test location is wrong.

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Looks good assuming tests pass

@mattip mattip merged commit 1ba4173 into numpy:master Oct 19, 2018
@mattip
Copy link
Member

mattip commented Oct 19, 2018

Thanks @kngwyu

@kngwyu kngwyu deleted the fix-resize-error branch October 19, 2018 15:47
@charris charris mentioned this pull request Nov 15, 2019
@charris
Copy link
Member

charris commented Nov 15, 2019

Fails for wheel tests on Python 3.7. See here.

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.

4 participants