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

Support "const" modifier on fused types #1772

Closed
deasmhumhna opened this issue Jul 15, 2017 · 10 comments · Fixed by #3118
Closed

Support "const" modifier on fused types #1772

deasmhumhna opened this issue Jul 15, 2017 · 10 comments · Fixed by #3118

Comments

@deasmhumhna
Copy link

cdef void c_encode_by_bits(const npy_intp n, const npy_intp m, const long * a, 
                           const long bitshift, integer * out) nogil:
    cdef npy_intp i, j 
    cdef long shift
    cdef integer value

    for i in range(n):
        value = 0
        shift = 0
        for j in range(m):
            value += a[i * m + j] << shift
            shift += bitshift
        out[i] = value
            
cdef void c_decode_by_bits(const npy_intp n, const npy_intp m, 
                           const integer * a, const long bitshift, 
                           long * out) nogil:
    cdef npy_intp i, j, k
    cdef long shift, mask = (1 << bitshift) - 1
    
    for i in range(n):
        shift = 0
        for j in range(m):
            out[i * m + j] = (<long> a[i] >> shift) & mask 
            shift += bitshift
ctypedef fused integer:
    char
    unsigned char
    short
    unsigned short
    int
    unsigned int
    long
    unsigned long
[1/1] Cythonizing locality/helpers/encode.pyx
Traceback (most recent call last):
  File "build.py", line 99, in <module>
    ext_modules = cythonize(cy_extensions) + c_extensions,
  File "/usr/lib/python3.6/site-packages/Cython/Build/Dependencies.py", line 934, in cythonize
    cythonize_one(*args)
  File "/usr/lib/python3.6/site-packages/Cython/Build/Dependencies.py", line 1039, in cythonize_one
    result = compile([pyx_file], options)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/Main.py", line 686, in compile
    return compile_multiple(source, options)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/Main.py", line 664, in compile_multiple
    result = run_pipeline(source, options, context=context)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/Main.py", line 494, in run_pipeline
    err, enddata = Pipeline.run_pipeline(pipeline, source)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/Pipeline.py", line 340, in run_pipeline
    data = phase(data)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/Pipeline.py", line 53, in generate_pyx_code_stage
    module_node.process_implementation(options, result)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/ModuleNode.py", line 137, in process_implementation
    self.generate_c_code(env, options, result)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/ModuleNode.py", line 365, in generate_c_code
    self.body.generate_function_definitions(env, code)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/Nodes.py", line 436, in generate_function_definitions
    stat.generate_function_definitions(env, code)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/Nodes.py", line 436, in generate_function_definitions
    stat.generate_function_definitions(env, code)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/Nodes.py", line 1754, in generate_function_definitions
    self.generate_function_header(code, with_pymethdef=with_pymethdef)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/Nodes.py", line 2466, in generate_function_header
    arg_decl = arg.declaration_code()
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/PyrexTypes.py", line 3050, in declaration_code
    return self.type.declaration_code(self.cname, for_display)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/PyrexTypes.py", line 2372, in declaration_code
    for_display, dll_linkage, pyrex)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/PyrexTypes.py", line 1475, in declaration_code
    return self.const_base_type.declaration_code("const %s" % entity_code, for_display, dll_linkage, pyrex)
  File "/usr/lib/python3.6/site-packages/Cython/Compiler/PyrexTypes.py", line 1533, in declaration_code
    raise Exception("This may never happen, please report a bug")

Replacing "integer" with "long" stops the exception.

@scoder
Copy link
Contributor

scoder commented Jul 18, 2017

I think the problem is the const integer. Declaring fused types as const after the fact seems unsupported. I would consider that a bug, constifying a fused type usage seems reasonable to me.

@scoder
Copy link
Contributor

scoder commented Jul 18, 2017

Here is a (non-minimal) test that fails to compile for me but should probably be allowed (can go into tests/run/fused_types.pyx):

cdef fused ints_t:
    int
    long

cdef ints_t cdef_const_fused(const ints_t *i):
    return i[0] + 2

def const_fused(const ints_t i):
    """
    >>> const_fused(5)
    12
    """
    return i + cdef_const_fused(&i)

@scoder scoder changed the title "Exception: This may never happen, please report a bug" (presumably) when using shift operator on fused type Support "const" modifier on fused types Jul 21, 2017
@lesteve
Copy link

lesteve commented Mar 8, 2018

For the record, I hit the exact same error when I tried to use const memoryview (from #1869) + fused types. To reproduce:

cdef fused floating1d:
    float[:]
    double[:]

def test(const floating1d arr):
    pass

As hinted by #1772 (comment), a work-around is to use const inside the fused type definition like this:

cdef fused const_floating1d:
    const float[:]
    const double[:]


def test(const_floating1d arr):
    pass

@lesteve
Copy link

lesteve commented Mar 12, 2018

As hinted by #1772 (comment), a work-around is to use const inside the fused type definition like this:

As noted in #2141 this work-around has some limitations because the const memoryview and the non-const memoryview types are seen as independent so that the function signature will be generated for all possible combination of types. For example:

from cython cimport floating
 
 
 cdef fused const_floating1d:
     const float[:]
     const double[:]
 
 
 def test(const_floating1d ro, floating[:] rw):
     rw[:] = ro
Error compiling Cython file:
------------------------------------------------------------
...
    const float[:]
    const double[:]


def test(const_floating1d ro, floating[:] rw):
    rw[:] = ro
           ^
------------------------------------------------------------

test.pyx:10:12: Different base types for memoryviews (const float, double)

@padix-key
Copy link
Contributor

padix-key commented Mar 16, 2018

I have another issue with memoyviews of fused types with const types. Here is a small code example:

cimport cython
cimport numpy as np
import numpy as np

ctypedef np.int8_t int8
ctypedef np.int16_t int16

ctypedef fused FusedType:
    const int8
    const int16
def func(FusedType[:] array):
    pass

The Cython compiler raises this error:

Error compiling Cython file:
------------------------------------------------------------
...
    cdef __Pyx_memviewslice memslice
    cdef Py_ssize_t itemsize
    cdef bint dtype_signed
    cdef char kind
    itemsize = -1
    cdef bint ___pyx_const int8_is_signed
                          ^
------------------------------------------------------------

(tree fragment):16:27: Syntax error in C variable declaration

This does only happen with ctypedef defined types, this problem does not occur with native types (char, short). Furthermore, this problem also diappears when a simple variable, rather than a memoryview, is used.

The error message suggests that const int8 is treated entirely as type name, which is clearly wrong, since const is a modifier.

I am using Cython version 0.28.

@scoder
Copy link
Contributor

scoder commented Mar 27, 2018

It looks like this is not difficult but a bit of work. Basically, in FusedNode.py, the fused_base_types must be generated without const (i.e. if type.is_const: type = type.const_base_type), but then the specialisation afterwards must look into const types to see if they are fused, and then specialise the type into the corresponding const type again. Not sure if this can be done locally in PyrexTypes.py or if it requires some non-local (but probably repetitive) code changes.
PR welcome.

@jakirkham
Copy link
Contributor

One challenge with the workaround is declaring an array or value within the function that matches the fused type without being constant.

@jakirkham
Copy link
Contributor

While playing with a workaround similar to @lesteve's, came up with the following tweak.

To ensure interoperability between the two types, we can add an if check that compares the two types. Interestingly the const_floating will drop the const part of its definition here. So a direct is/is not comparison works.

All that is left is to bail out of bad prototypes preferably with some error that makes the user aware. To that extent, we raise a TypeError explaining the type mismatch to the user. Cython also recognizes the rest of the function is unreachable so does not generate the rest of it (avoiding any compile errors on the way).

Only downside is Cython will warn the rest of the function is unreachable. We can fix this by using a decorator on the function to disable the unreachable warning. Though this will suppress any other such warnings in the function. The other alternatively would be to move the rest of the code into an else branch of type check condition. Maybe other options exist here?

This is pretty similar to how static_assert works in C++. So this is a common way of solving this sort of problem. Maybe Cython can even leverage it under the hood?

cimport cython
from cython cimport floating


cdef fused const_floating:
    const float
    const double


@cython.warn.unreachable(False)
cpdef void my_safe_float_copy(floating[:] a, const_floating[:] b):
    if floating is not const_floating:
        raise TypeError("Types of a and b don't match")

    a[:] = b.copy()

@scoder
Copy link
Contributor

scoder commented Aug 23, 2018

Maybe Cython can even leverage it under the hood?

No, we should avoid generating the functions entirely. My comment above explains what needs to be done.

@rth
Copy link

rth commented Sep 10, 2018

We spend half a day investigating this issue with @lesteve at EuroScipy. Posting a (partial) status update in case it might be helpful.

A (possibly) minimal unit test that fails can be found in this branch, where to produce an error it is sufficient to run,

python -m Cython.Build.Cythonize cython/tests/memoryview/const_fused_type_memoryview.pyx

or alternatively,

python runtests.py -T 1772

(though the traceback in the latter case is less detailed).

Below are a few additional observation (from someone with no formal CS background or in depth understanding how Cython works, and possibly I missed some contribution docs),

  • Looking at the generated C code,

    • the difference between float and const float declaration is just a few additional "const" declaration
    • the difference between float and floating (fused float type) is fairly large (though this has been tested with memoryviews: with plain scalars, it might not longer be the case)

    so the probable expected result of the PR that would fix this issue might be a slight modification of the resulting C code.

  • in the overall processing pipeline the processing step which is probably affected is AnalyseDeclarationsTransform (cf @scoder's above comment Support "const" modifier on fused types #1772 (comment)).

  • we had some trouble specifying breakpoints for pdb in the code as it appears that some source files are cythonized. Using git clean -xdf could be a possible workaround.

There are probably much better ways of approaching this issue for someone with better knowledge of Cython code base.

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

Successfully merging a pull request may close this issue.

6 participants