-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
remove refcount semantics, now a.resize() almost always requires refc… #8050
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
@@ -4080,7 +4081,10 @@ def test___array__(self): | |||
class TestResize(TestCase): | |||
def test_basic(self): | |||
x = np.array([[1, 0, 0], [0, 1, 0], [0, 0, 1]]) | |||
x.resize((5, 5)) | |||
if IS_PYPY: |
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.
Out of curiousity, I noticed the import also defines HAS_REFCOUNT
, is this reflected in the C-api so that this could be not PyPy specific but in principle good for all non-refcounted pythons?
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.
HAS_REFCOUNT reflects sys.getrefcount() where this pull request deals with pyobj->ob_refcnt not being incremented at the same places in CPython and PyPy. I'm not sure the two concepts are equivalent, I could imagine a garbage collection strategy that cannot count overall refcounts but can track single-item assignments, or visa-versa
@juliantaylor @mattip I'm wondering if #7997 is compatible with PyPy. |
Looks like you picked up a stray commit somewhere, probably merged master into your branch. |
@charris thanks, should be fixed now |
y = np.array([123456789e199], dtype=np.float64) | ||
y.resize((0, n)) | ||
if IS_PYPY: | ||
y.resize((0, n),refcheck=False) |
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.
Space missing, but will merge without a fix ;).
Unless I missed something big or someone else wants to look over (don't know the PyPy stuff much), this all looks good to me and I will merge it tomorrow. |
@seberg thanks, I will fix up that space in an inevitable upcoming pull request, there are still around 30 failing tests so chances are there will be another opportunity |
Thanks a lot, putting this in. Hopefully, your PyPy support gets better. Just thought of another Nitpick (its a sport ;)). Not actually sure, but I think we often make it |
The refcount semantics on PyPy do not provide a reliable way to determine if other references to the buffer are alive. So I propose to more stringently require refcheck=False for a.resize(). I also undefined the refcount-related entries in the NumPy headers, so downstream issues will be detected earlier.
Note that with this commit, NumPy now fails only 33 tests on a nightly PyPy.