Skip to content

STY: Boolean values for bint variables #33008

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

Conversation

ShaharNaveh
Copy link
Member

  • 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
@jbrockmendel
Copy link
Member

Looks fine. Any reason to suspect a performance impact? (for this one id look at the generated C code and see if it suddenly became more verbose)

@ShaharNaveh
Copy link
Member Author

Looks fine. Any reason to suspect a performance impact? (for this one id look at the generated C code and see if it suddenly became more verbose)

I looked a the generated C-code, there is no difference (except for the comments that show the equivalent code at the pyx file), this change is purely a stylistic change.

@WillAyd
Copy link
Member

WillAyd commented Mar 25, 2020

I looked a the generated C-code, there is no difference (except for the comments that show the equivalent code at the pyx file), this change is purely a stylistic change.

Can you run benchmarks to be sure?

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Mar 25, 2020

I looked a the generated C-code, there is no difference (except for the comments that show the equivalent code at the pyx file), this change is purely a stylistic change.

Can you run benchmarks to be sure?

Sure!
@WillAyd Can you please guide to which asv benchmarks you'd like to see for this PR?

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

jreback commented Mar 25, 2020

also you can run cython -a to generate the annotate code to see if there is any difference (alt just recompile); if the c-code is the same then we are good.

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Mar 26, 2020

also you can run cython -a to generate the annotate code to see if there is any difference (alt just recompile); if the c-code is the same then we are good.

@jreback I have done both, and the result are the same.


I also did a small POC, what I did was:

Created a file called foo.pyx and it's contents:

def f(bint VAR_NAME=0):
    pass

then ran:

cython -3 foo.pyx
mv foo.c foo_ZERO.c   # So future diff won't have diffs related to the file name.
rm -f foo.*                     # Was afraid that cython uses same file name as cache or something crazy like that

then I rewrote foo.pyx, and it's contents:

def f(bint VAR_NAME=False):
    pass

and then I ran:

cython -3 foo.pyx
mv foo.c foo_FALSE.c

and then I checked via vimdiff:

vimdiff foo_ZERO.c foo_FALSE.c

generated_c_code


I have also included the two files generated in the POC

foo.zip

@jorisvandenbossche jorisvandenbossche merged commit 218cc30 into pandas-dev:master Mar 26, 2020
@jorisvandenbossche
Copy link
Member

Thanks!

@ShaharNaveh ShaharNaveh deleted the STY-bint-boolean-not-int-tslib-aggregations-writers branch March 26, 2020 15:40
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.

5 participants