-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
FIX fixes memory leak seen in PyPy in C losses #27670
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
PyPy tests go until 98%, almost there! |
Is the plan to merge this even before we get to 100%? From a quick look at the diff it looks uncontroversial. I think technically it changes the API of the loss functions (they now return @lorentzenchr do you want to have a look and think about this (I think you wrote a lot of this code?)? |
We can remove those return statements, I guess the only use is testing and the Python loss.py submodule. The docstrings need to be modified, too. |
I assume the fortran style is needed to make sure we re-use the same array/memory each time and results in a significant performance improvement. Is that the right assumption? I don't know enough about the history of this code and the discussions around it, so if this has been discussed before: I don't want to re-start an old discussion, just check that someone thought about this topic. Because I agree having a more pythonic API would be nicer. |
We want to achieve ufunc like behaviour, e.g. |
I also like the fact that we allocate the memory via NumPy in Python usually and just interact with it. We don't have to free the buffer ourselves. |
Co-authored-by: Tim Head <betatim@gmail.com>
Co-authored-by: Tim Head <betatim@gmail.com>
Merging, thanks! Let's see if the PyPy build manages to complete from time to time with this fix 🤞 ! |
Co-authored-by: Tim Head <betatim@gmail.com>
partially addresses #27662
Avoid to use
np.asarray
that creates a reference to the memory view and seems to not be garbage collected in PyPy.