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
@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
Contributor 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
Contributor 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
Contributor 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