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

compilation failed with the new cython version (0.28) #1435

Closed
skoudoro opened this Issue Feb 16, 2018 · 13 comments

Comments

Projects
None yet
3 participants
@skoudoro
Member

skoudoro commented Feb 16, 2018

Description

As you can see on this build, the new Cython version (0.28) does not allow to use memoryviews as struct members.

In this discussion, it is hard to say if it will be something permanent or not.

BTW, We have to find a workaround.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 20, 2018

If I remember correctly, this comment (cython/cython#1453 (comment)) is exactly what we are doing, i.e. manually nulling the memview at creation and freeing it in dealloc.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 20, 2018

Check the travis error the message is clear. Memory views not allowed as struct members.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 20, 2018

And even if you nullify it the possibility for memory leak is still there.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 20, 2018

Basically, they are not allowing the code to compile when we add memviews inside structs. We can suggest to reconsider not changing this option.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 20, 2018

cdef struct Centroid:
    Data2D features # float* features
    int size
    float[6] aabb
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 20, 2018

Perhaps changing from Data2D to float * will do the job? Let us know if you have time to hangout on this. We need to coordinate.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 20, 2018

Otherwise perhaps we can change from struct to class for structures with memviews?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 20, 2018

Okay @MarcCote the class idea was not good.
But I think we can make something like this

cdef struct Centroid2:
    float * features #using pointer rather than memview
    int size

and then have functions to move from pointer to memview and memview to pointer

def memview_to_pointer(float [:, ::1] mv):
    cdef Centroid2 c
    c.features = <float *> &mv[0, 0]

def pointer_to_memview(size_t N, size_t M):

    cdef Centroid2 c
    c.features = <float *>PyMem_Malloc(N * M * sizeof(float))  
    cdef float[:, ::1] mv = <float[:N, :M]> c.features
    return mv
    
def free_memview(float [:, ::1] mv): #not working
    if mv._data is not NULL:
        PyMem_Free(mv._data) 
@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 21, 2018

I like the Centroid2 struct idea. Do we really need the conversion functions, though?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 21, 2018

The data are coming in as memviews or numpy arrays. And you use 2D memviews internally too. So... probably yes. But if you think otherwise we would love to hear an alternative.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 22, 2018

I don't have a precise idea on how to do it. I wasn't able to install cython 0.28 on my machine. Is 0.28 officially released?

@skoudoro

This comment has been minimized.

Member

skoudoro commented Feb 22, 2018

This is a future release @MarcCote. Create a new environment and you can install it via :

pip install --find-links=https://5cf40426d9f06eb7461d-6fe47d9331aba7cd62fc36c7196769e4.ssl.cf2.rackcdn.com --find-links=https://7933911d6844c6c53a7d-47bd50c35cd79bd838daf386af554a83.ssl.cf2.rackcdn.com --pre cython numpy scipy h5py nibabel
@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 3, 2018

fix by #1434, closing !

@skoudoro skoudoro closed this Mar 3, 2018

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