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

Return numpy arrays instead of memory views from cython functions #826

Merged
merged 2 commits into from Jan 22, 2016

Conversation

Projects
None yet
3 participants
@omarocegueda
Contributor

omarocegueda commented Jan 4, 2016

Workaround to prevent this kind of memory leaks:
https://github.com/omarocegueda/cython_leak_repro/blob/master/memory_leak.ipynb
which occur when using memory views allocated and returned from cython functions

@arokem

This comment has been minimized.

Member

arokem commented Jan 5, 2016

I've pushed a branch with this change and the appveyor config here:

https://github.com/arokem/dipy/tree/omarocegueda-wrap_memviews-appveyor

It's running an appveyor build over here:
https://ci.appveyor.com/project/arokem/dipy/build/1.0.576

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Jan 5, 2016

Awesome! let's see what it says =)

@arokem

This comment has been minimized.

Member

arokem commented Jan 5, 2016

Still no joy:
https://ci.appveyor.com/project/arokem/dipy/build/1.0.576/job/4j8cy33yvdesmd66

On Tue, Jan 5, 2016 at 9:41 AM, Omar Ocegueda notifications@github.com
wrote:

Awesome! let's see what it says =)


Reply to this email directly or view it on GitHub
#826 (comment).

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 18, 2016

@omarocegueda rather than using np.array(memview) it would be interesting to check if it is more advantageous to use np.asarray(memview) see here
http://stackoverflow.com/questions/20978377/cython-convert-memory-view-to-numpy-array
By using asarray you will use less memory as the memview is directly being transformed into an array without copying extra memory.
See the difference here
http://stackoverflow.com/questions/14415741/numpy-array-vs-asarray

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 22, 2016

@omarocegueda can you update this PR? It will be nice to get rid of the memory leaks (or any copies) in this release.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Jan 22, 2016

No problem!, now using np.asarray(...). This will eliminate the memory leaks, but not yet the issue on appveyor.

@arokem

This comment has been minimized.

Member

arokem commented Jan 22, 2016

That's OK. We'll figure out the appveyor thing on another branch. For the time being, @Garyfallidis - is this ready to be merged?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 22, 2016

Yes!

Garyfallidis added a commit that referenced this pull request Jan 22, 2016

Merge pull request #826 from omarocegueda/wrap_memviews
Return numpy arrays instead of memory views from cython functions

@Garyfallidis Garyfallidis merged commit 73bf257 into nipy:master Jan 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Jan 22, 2016

Thank you guys! =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment