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

Add support of const fused type memory views #3118

Merged
merged 7 commits into from
Sep 10, 2019
Merged

Conversation

t20100
Copy link
Contributor

@t20100 t20100 commented Aug 30, 2019

This PR is related to #1772 and is an attempt at supporting const fused type memory views such as:

cimport cython
def sum(const cython.floating[:] array):
    cdef cython.floating sum = 0
    for v in array:
        sum += v
    return sum

Cython internals are new to me, so I don't know how far this is from a proper solution, but it's working fine from what I tested.
Let me know if it is worth spending more time on it and what can be improved.

One remark: In case of function signature like def test(const cython.floating[:] a, cython.foating[:] b) and mismatching input floating types, the exception is:
TypeError: No matching signature found
while without const it is: ValueError: Buffer dtype mismatch, expected 'float' but got 'double'

Closes #1772.

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I left some comments.

Cython/Compiler/PyrexTypes.py Outdated Show resolved Hide resolved
Cython/Compiler/PyrexTypes.py Outdated Show resolved Hide resolved
tests/run/fused_types.pyx Show resolved Hide resolved
@t20100
Copy link
Contributor Author

t20100 commented Sep 6, 2019

PR updated to use CConstOrVolatileType: change is now a single line.
More tests still need to be added.

@scoder
Copy link
Contributor

scoder commented Sep 6, 2019

Change looks perfect, but yes, more tests for const usage with (non-memoryview) fused types are needed. I can't currently find the word const anywhere in fused_types.pyx (probably because it came in later as a feature), so the combination of both features is very much undertested.

@t20100
Copy link
Contributor Author

t20100 commented Sep 9, 2019

Added a few tests of const fused types usage without memory view.
Tell me if other use cases of const (and which one?) needs to be tested.

@scoder scoder added this to the 3.0 milestone Sep 9, 2019
@scoder scoder merged commit ff8e254 into cython:master Sep 10, 2019
@scoder
Copy link
Contributor

scoder commented Sep 10, 2019

Thanks!

@t20100 t20100 deleted the const_fused branch September 10, 2019 14:50
@lesteve
Copy link

lesteve commented Sep 10, 2019

Wow nice! Thanks a lot for this, this is something we needed in scikit-learn for some time. I'll try to get someone to try this out in scikit-learn and give feed-back.

jjerphan added a commit to jjerphan/scikit-learn that referenced this pull request Aug 25, 2023
This problem was addressed by cython/cython#3118
which first was released in Cython 0.29.33:
https://cython.readthedocs.io/en/latest/src/changes.html#id80

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
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.

Support "const" modifier on fused types
3 participants