Skip to content

STY: Boolean values for bint variables #33005

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

Merged

Conversation

ShaharNaveh
Copy link
Member

@ShaharNaveh ShaharNaveh commented Mar 25, 2020

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

asv benchmarks: to show that performance is not changed with the stylistic changes)

All benchmarks:

       before           after         ratio
     [28e0f18a]       [6f1fd78d]
     <master>         <STY-bint-boolean-not-int-join>
         21.4±1ms       19.8±0.4ms     0.92  join_merge.Join.time_join_dataframe_index_multi(False)
       24.9±0.3ms       23.6±0.3ms     0.95  join_merge.Join.time_join_dataframe_index_multi(True)
       14.5±0.3ms       14.0±0.3ms     0.97  join_merge.Join.time_join_dataframe_index_shuffle_key_bigger_sort(False)
       17.6±0.6ms       16.7±0.5ms     0.95  join_merge.Join.time_join_dataframe_index_shuffle_key_bigger_sort(True)
       14.4±0.3ms       13.8±0.2ms     0.95  join_merge.Join.time_join_dataframe_index_single_key_bigger(False)
       17.8±0.7ms       16.4±0.5ms     0.92  join_merge.Join.time_join_dataframe_index_single_key_bigger(True)
       13.5±0.3ms       13.1±0.3ms     0.97  join_merge.Join.time_join_dataframe_index_single_key_small(False)
       14.5±0.1ms       13.9±0.5ms     0.96  join_merge.Join.time_join_dataframe_index_single_key_small(True)
       2.86±0.08s       2.94±0.05s     1.03  join_merge.JoinIndex.time_left_outer_join_index
         433±70μs         428±50μs     0.99  join_merge.JoinNonUnique.time_join_non_unique_equal

@@ -78,7 +78,7 @@ def inner_join(const int64_t[:] left, const int64_t[:] right,

@cython.boundscheck(False)
def left_outer_join(const int64_t[:] left, const int64_t[:] right,
Py_ssize_t max_groups, sort=True):
Py_ssize_t max_groups, bint sort=True):
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only place where I'm adding bint

@ShaharNaveh ShaharNaveh changed the title STY: bint boolean values rather than integer values STY: Boolean values for bint variables Mar 25, 2020
@ShaharNaveh ShaharNaveh added the Code Style Code style, linting, code_checks label Mar 25, 2020
@jorisvandenbossche
Copy link
Member

I am not sure asv is always appropriate for changes that might only give a small change in performance. Can you maybe check the generated code to ensure that no more work is being done (unless this is documented in the cython docs to work fine?)

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Mar 25, 2020

I am not sure asv is always appropriate for changes that might only give a small change in performance. Can you maybe check the generated code to ensure that no more work is being done (unless this is documented in the cython docs to work fine?)

@jorisvandenbossche I have checked the sources of the generated C-files. there is no code change at all (except for the comments displaying the equivalent code at the .pyx file), this is purely stylistic change.

@jorisvandenbossche
Copy link
Member

Super, thanks for checking!

@jbrockmendel
Copy link
Member

Hah, I just made the same suggestion in the next PR

@WillAyd WillAyd added this to the 1.1 milestone Mar 25, 2020
@WillAyd WillAyd merged commit b6fdc13 into pandas-dev:master Mar 25, 2020
@WillAyd
Copy link
Member

WillAyd commented Mar 25, 2020

Thanks @MomIsBestFriend

@ShaharNaveh ShaharNaveh deleted the STY-bint-boolean-not-int-join branch March 25, 2020 23:54
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