manodeep / Corrfunc

⚡️⚡️⚡️Blazing fast correlation functions on the CPU.
https://corrfunc.readthedocs.io
MIT License
163 stars 50 forks source link

Ref count leak on weights arrays #181

Closed lgarrison closed 5 years ago

lgarrison commented 5 years ago

General information

Issue description

Calling DD_extn permanently increases the reference count on the weights array if the array is already contiguous and C order. This is because PyArray_Newshape increases the ref count on the target if it returns a view. This is probably intended behavior, but it's cumbersome to check whether a view was returned and then hold on to that information for the whole scope; we don't need to retain the pointer to the original object for any other reason. This seems like it would be a common problem, but I haven't found mention of it elsewhere. More likely I'm just misunderstanding something!

See: https://github.com/manodeep/Corrfunc/blob/master/theory/python_bindings/_countpairs.c#L1212

Minimal working example

import numpy as np

N_d = 10000
Lbox = 250.

x,y,z = np.random.uniform(0.,Lbox, size=(3,N_d))
w = np.random.uniform(0.,Lbox, size=N_d)

bins = np.logspace(-1,1.,25)

import sys
print(sys.getrefcount(w))  # prints 2

from Corrfunc._countpairs import countpairs as DD_extn

results = DD_extn(1,16,'adsf',x,y,z,w,weight_type='pair_product')

print(sys.getrefcount(w))  # prints 3

Solution

I think we'll just stop using PyArray_Newshape. This means that the weights arrays must be passed as 2D (nweight,npart) arrays always, which our Python wrappers can handle.

This change will not be user-facing since users are not supposed to call the Python C extensions directly.

manodeep commented 5 years ago

I wonder if this is related to #162

manodeep commented 5 years ago

@lgarrison is this solved now?

lgarrison commented 5 years ago

Yes; I got the syntax wrong for closing multiple issues from a commit message!