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

remove refcount semantics, now a.resize() almost always requires refc… #8050

Merged
merged 1 commit into from
Sep 19, 2016
Merged

remove refcount semantics, now a.resize() almost always requires refc… #8050

merged 1 commit into from
Sep 19, 2016

Conversation

mattip
Copy link
Member

@mattip mattip commented Sep 12, 2016

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.

@@ -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:
Copy link
Member

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?

Copy link
Member Author

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

@charris
Copy link
Member

charris commented Sep 13, 2016

@juliantaylor @mattip I'm wondering if #7997 is compatible with PyPy.

@mattip
Copy link
Member Author

mattip commented Sep 13, 2016

@charris #7997 currently has no chance of working on PyPy. I added a comment there. We would need to support very specific CPython refcounting semantics and the whole backtrace idea.

@charris
Copy link
Member

charris commented Sep 16, 2016

Looks like you picked up a stray commit somewhere, probably merged master into your branch.

@mattip
Copy link
Member Author

mattip commented Sep 17, 2016

@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)
Copy link
Member

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 ;).

@seberg
Copy link
Member

seberg commented Sep 18, 2016

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.

@mattip
Copy link
Member Author

mattip commented Sep 18, 2016

@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

@seberg
Copy link
Member

seberg commented Sep 19, 2016

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 #endif /* repeat condition */.

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.

None yet

3 participants