Skip to content

Add support of const fused type memory views#3118

Merged
scoder merged 7 commits intocython:masterfrom
t20100:const_fused
Sep 10, 2019
Merged

Add support of const fused type memory views#3118
scoder merged 7 commits intocython:masterfrom
t20100:const_fused

Conversation

@t20100
Copy link
Copy Markdown
Contributor

@t20100 t20100 commented Aug 30, 2019 •
edited by scoder
Loading

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.

scoder
scoder reviewed Sep 6, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread Cython/Compiler/PyrexTypes.py Outdated
Comment thread Cython/Compiler/PyrexTypes.py Outdated
Comment thread tests/run/fused_types.pyx
@t20100
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

scoder commented Sep 10, 2019

Thanks!

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

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.

@lesteve lesteve mentioned this pull request Sep 10, 2019
Closed
@grlee77 grlee77 mentioned this pull request Sep 16, 2019
Open
@ogrisel ogrisel mentioned this pull request Sep 17, 2019
Merged
6 tasks
@thomasjpfan thomasjpfan mentioned this pull request Oct 28, 2019
Closed
@kernc kernc mentioned this pull request Feb 5, 2020
Closed
@ivirshup ivirshup mentioned this pull request Mar 26, 2020
Open
@clarkzinzow clarkzinzow mentioned this pull request Aug 15, 2020
Closed
2 tasks
da-woods pushed a commit to da-woods/cython that referenced this pull request Oct 9, 2022
@t20100 @da-woods
Add support of const fused type memory views (cythonGH-3118)
2d4a24c
scoder pushed a commit that referenced this pull request Jan 4, 2023
@da-woods @t20100
Add support of const fused type memory views (GH-3118) (GH-5076)
08e4478 
Backport of #3118
Fixes #3783

Co-authored-by: Thomas VINCENT <thomas.vincent@esrf.fr>
@scoder scoder modified the milestones: 3.0, 0.29.33 Jan 4, 2023
jjerphan added a commit to jjerphan/scikit-learn that referenced this pull request Aug 25, 2023
@jjerphan
DOC Remove outdated comments regarding memoryviews
61e6fa2 
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>
@jjerphan jjerphan mentioned this pull request Aug 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scoder scoder scoder left review comments

Assignees

No one assigned

Labels

enhancement Type Analysis

Projects

None yet

Milestone

0.29.33

Development

Successfully merging this pull request may close these issues.

Support "const" modifier on fused types

3 participants

@t20100 @scoder @lesteve