Skip to content

Conversation

ShaharNaveh
Copy link
Contributor

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@ShaharNaveh ShaharNaveh added the Code Style Code style, linting, code_checks label Mar 25, 2020
# is a general, O(max(len(values), len(binner))) method.
@cython.boundscheck(False)
@cython.wraparound(False)
def generate_bins_dt64(ndarray[int64_t] values, const int64_t[:] binner,
Copy link
Member

Choose a reason for hiding this comment

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

can values here be `const int64_t[:]``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, cython raises errors

[1/1] Cythonizing pandas/_libs/lib.pyx

Error compiling Cython file:
------------------------------------------------------------
...
        int64_t l_bin, r_bin, nat_count
        bint right_closed = closed == 'right'

    nat_count = 0
    if hasnans:
        mask = values == NPY_NAT
                     ^
------------------------------------------------------------

pandas/_libs/lib.pyx:707:22: Invalid types for '==' (const int64_t[:], int64_t)
warning: pandas/_libs/lib.pyx:709:24: Index should be typed for more efficient access

Error compiling Cython file:
------------------------------------------------------------
...

    nat_count = 0
    if hasnans:
        mask = values == NPY_NAT
        nat_count = np.sum(mask)
        values = values[~mask]
                      ^
------------------------------------------------------------

pandas/_libs/lib.pyx:709:23: Cannot convert 'int64_t' to memoryviewslice
Traceback (most recent call last):
  File "setup.py", line 791, in <module>
    setup_package()
  File "setup.py", line 761, in setup_package
    ext_modules=maybe_cythonize(extensions, compiler_directives=directives),
  File "setup.py", line 534, in maybe_cythonize
    return cythonize(extensions, *args, **kwargs)
  File "/home/bummy/miniconda3/envs/pandas-dev/lib/python3.7/site-packages/Cython/Build/Dependencies.py", line 1102, in cythonize
    cythonize_one(*args)
  File "/home/bummy/miniconda3/envs/pandas-dev/lib/python3.7/site-packages/Cython/Build/Dependencies.py", line 1225, in cythonize_one
    raise CompileError(None, pyx_file)
Cython.Compiler.Errors.CompileError: pandas/_libs/lib.pyx

@WillAyd
Copy link
Member

WillAyd commented Mar 25, 2020

Same comment as #33008 - a lot of the Cython stuff is performance critical so really want to be sure

@jreback jreback added this to the 1.1 milestone Mar 25, 2020
@jreback
Copy link
Contributor

jreback commented Mar 26, 2020

can you just verify that the same code is generated here as well, otherwise lgtm.

@ShaharNaveh
Copy link
Contributor Author

@jreback I checked and the generated C code, is identical. (Only diff is in the comments)

@jreback jreback merged commit 754caad into pandas-dev:master Mar 26, 2020
@jreback
Copy link
Contributor

jreback commented Mar 26, 2020

thanks @MomIsBestFriend

I suppose if we had a code check to catch this would be great, but might not be so easy to check

@ShaharNaveh ShaharNaveh deleted the STY-bint-boolean-not-int-lib branch March 29, 2020 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Code Style Code style, linting, code_checks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants