-
-
Notifications
You must be signed in to change notification settings - Fork 11.8k
BUG: Fix resize when it contains references #29970
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
Conversation
mhvk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one missing assert.
numpy/_core/tests/test_multiarray.py
Outdated
| test_obj = object() | ||
| expected = sys.getrefcount(test_obj) | ||
| a = np.array([test_obj, test_obj, test_obj], dtype=dtype) | ||
| a.size == 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing assert here!
For object arrays, I think it pays to just be correct -- and everything is slow anyway, |
| assert a.size == 2 | ||
| del a | ||
| # if all is well, then we reclaimed all references | ||
| assert sys.getrefcount(test_obj) == expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not available on pypy
139c755 to
b847956
Compare
mhvk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
|
Thanks @seberg thanks for the nudge. |
BUG: Fix resize when it contains references (#29970)
Small fix for resize with objects, which was always broken.
@mhvk one little thing is that we of course don't need to do this in all of the internal places in theory.
Closes gh-29887.