Clean Up C Warnings #31935
Merged
Clean Up C Warnings #31935
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WillAyd
commented
Feb 12, 2020
| @@ -15,7 +15,7 @@ DEF dROUNDS = 4 | |||
|
|
|||
|
|
|||
| @cython.boundscheck(False) | |||
| def hash_object_array(object[:] arr, object key, object encoding='utf8'): | |||
| def hash_object_array(ndarray[object] arr, object key, object encoding='utf8'): | |||
There was a problem hiding this comment.
I get the impression that memory views used in tandem with object will throw warnings cc @jbrockmendel
There was a problem hiding this comment.
that makes sense. this would also raise ATM if we were to pass a read-only ndarray[object], so a worthwhile change
| @@ -1173,8 +1174,8 @@ ctypedef fused out_t: | |||
|
|
|||
| @cython.boundscheck(False) | |||
| @cython.wraparound(False) | |||
| def diff_2d(ndarray[diff_t, ndim=2] arr, | |||
| ndarray[out_t, ndim=2] out, | |||
| def diff_2d(diff_t[:, :] arr, | |||
There was a problem hiding this comment.
Hmm tried but seems to crash the compiler. Traceback below
[1/1] Cythonizing pandas/_libs/algos.pyx
Error compiling Cython file:
------------------------------------------------------------
...
float64_t
@cython.boundscheck(False)
@cython.wraparound(False)
def diff_2d(const diff_t[:, :] arr,
^
------------------------------------------------------------
pandas/_libs/algos.pyx:1178:25: Compiler crash in AnalyseDeclarationsTransform
File 'ModuleNode.py', line 124, in analyse_declarations: ModuleNode(algos.pyx:1:0,
full_module_name = 'pandas._libs.algos')
File 'Nodes.py', line 431, in analyse_declarations: StatListNode(algos.pyx:1:0)
File 'Nodes.py', line 431, in analyse_declarations: StatListNode(algos.pyx:1178:0)
File 'Nodes.py', line 375, in analyse_declarations: CompilerDirectivesNode(algos.pyx:1178:0)
File 'Nodes.py', line 431, in analyse_declarations: StatListNode(algos.pyx:1178:0)
File 'Nodes.py', line 2887, in analyse_declarations: DefNode(algos.pyx:1178:0,
modifiers = [...]/0,
name = 'diff_2d',
num_required_args = 4,
outer_attrs = [...]/2,
py_wrapper_required = True,
reqd_kw_flags_cname = '0')
File 'Nodes.py', line 2925, in analyse_argument_types: DefNode(algos.pyx:1178:0,
modifiers = [...]/0,
name = 'diff_2d',
num_required_args = 4,
outer_attrs = [...]/2,
py_wrapper_required = True,
reqd_kw_flags_cname = '0')
File 'Nodes.py', line 1093, in analyse: MemoryViewSliceTypeNode(algos.pyx:1178:25,
name = 'memoryview')
Compiler crash traceback from this point on:
File "/Users/williamayd/miniconda3/envs/pandas-dev/lib/python3.7/site-packages/Cython/Compiler/Nodes.py", line 1093, in analyse
self.type = PyrexTypes.MemoryViewSliceType(base_type, axes_specs)
File "/Users/williamayd/miniconda3/envs/pandas-dev/lib/python3.7/site-packages/Cython/Compiler/PyrexTypes.py", line 625, in __init__
self.dtype_name = Buffer.mangle_dtype_name(self.dtype)
File "/Users/williamayd/miniconda3/envs/pandas-dev/lib/python3.7/site-packages/Cython/Compiler/Buffer.py", line 631, in mangle_dtype_name
return prefix + dtype.specialization_name()
File "/Users/williamayd/miniconda3/envs/pandas-dev/lib/python3.7/site-packages/Cython/Compiler/PyrexTypes.py", line 57, in specialization_name
common_subs = (self.empty_declaration_code()
File "/Users/williamayd/miniconda3/envs/pandas-dev/lib/python3.7/site-packages/Cython/Compiler/PyrexTypes.py", line 51, in empty_declaration_code
self._empty_declaration = self.declaration_code('')
File "/Users/williamayd/miniconda3/envs/pandas-dev/lib/python3.7/site-packages/Cython/Compiler/PyrexTypes.py", line 1582, in declaration_code
return self.const_base_type.declaration_code("const %s" % entity_code, for_display, dll_linkage, pyrex)
File "/Users/williamayd/miniconda3/envs/pandas-dev/lib/python3.7/site-packages/Cython/Compiler/PyrexTypes.py", line 1649, in declaration_code
raise Exception("This may never happen, please report a bug")
Exception: This may never happen, please report a bug
Traceback (most recent call last):
File "setup.py", line 784, in <module>
setup_package()
File "setup.py", line 754, in setup_package
ext_modules=maybe_cythonize(extensions, compiler_directives=directives),
File "setup.py", line 527, in maybe_cythonize
return cythonize(extensions, *args, **kwargs)
File "/Users/williamayd/miniconda3/envs/pandas-dev/lib/python3.7/site-packages/Cython/Build/Dependencies.py", line 1096, in cythonize
cythonize_one(*args)
File "/Users/williamayd/miniconda3/envs/pandas-dev/lib/python3.7/site-packages/Cython/Build/Dependencies.py", line 1219, in cythonize_one
raise CompileError(None, pyx_file)
Cython.Compiler.Errors.CompileError: pandas/_libs/algos.pyxThere was a problem hiding this comment.
huh, possibly a cython issue
pandas/_libs/algos.pyx
Outdated
| @@ -1179,7 +1180,7 @@ def diff_2d(diff_t[:, :] arr, | |||
| Py_ssize_t periods, int axis): | |||
| cdef: | |||
| Py_ssize_t i, j, sx, sy, start, stop | |||
| bint f_contig = arr.flags.f_contiguous | |||
| bint f_contig = PyBuffer_IsContiguous(arr, 'F') | |||
There was a problem hiding this comment.
neat. probably not worth the effort on our end, but itd be nice if cython automatically made this optimization
There was a problem hiding this comment.
tbh didn't look too much into the subsequent code; think could be cleaned up?
|
If you'd like to remove the |
|
Nice - I think that's a better solution |
|
Darn. Sorry, looks like my suggestion didn't work @WillAyd. I only compiled + tested the tests in |
|
I think the failure is from something else. Giving it a closer look |
jreback
reviewed
Feb 15, 2020
| @@ -56,7 +56,7 @@ cdef: | |||
| cdef inline int int_max(int a, int b): return a if a >= b else b | |||
| cdef inline int int_min(int a, int b): return a if a <= b else b | |||
|
|
|||
| cdef inline bint is_monotonic_start_end_bounds( | |||
| cdef bint is_monotonic_start_end_bounds( | |||
There was a problem hiding this comment.
hmm, why can't this also be in-line?
There was a problem hiding this comment.
This was the cause of the warning: pandas/_libs/window/aggregations.pyx:60:4: Buffer unpacking not optimized away. warning. @mroeschke suggested a potential inline approach that got rid of the warning and used memory views but caused test failures. Could still adjust a few things to make that work but I think without a larger effort this is the best option for now
There was a problem hiding this comment.
this is a highly optimized path
if u can show this doesn’t have any perf regressions then ok
|
@jreback here are the ASV results after this change. Mixed bag but generally my interpretation would be generally better performance |
|
@mroeschke of the asv results Will posted, are any of those benchmarks more important than others? i.e. are the few that are slower deal-breakers? |
|
None that are concerning IMO. And the Numba engine benchmarks improved which I'm happy about :) |
mroeschke
approved these changes
Feb 17, 2020
|
LGTM |
|
thanks @WillAyd |
roberthdevries
pushed a commit
to roberthdevries/pandas
that referenced
this pull request
Mar 2, 2020
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Getting closer to no warnings (at least with clang). Changes in this PR get rid of the following warnings:
Only a handful left after this