GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-37229: Add compare_function to bisect functions by FortStatement · Pull Request #13970 · python/cpython

Conversation

@FortStatement

@FortStatement FortStatement commented Jun 11, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All of bisect's functions (insort_{left,right}, bisect_{left,right}) can only use comparison of objects via __lt__.
They should support providing a custom comparison function.

https://bugs.python.org/issue37229

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cirosantilli and jungsu-kwon reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@FortStatement
FortStatement requested a review from rhettinger as a code owner June 11, 2019 10:40
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jun 11, 2019
@bedevere-bot bedevere-bot added the awaiting review label Jun 11, 2019
@asvetlov

asvetlov commented Jun 11, 2019

Copy link
Copy Markdown
Contributor

Please fix bpo number

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FortStatement FortStatement changed the title bpo-345216: Add compare_function to bisect functions bpo-37229: Add compare_function to bisect functions Jun 11, 2019
GPery
bpo-345216: Add compare_function to bisect functions
056e27a
@FortStatement
FortStatement force-pushed the bisect-custom-compare branch from d2f526e to 056e27a Compare June 11, 2019 10:53
@remilapeyre

remilapeyre commented Jun 11, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Hi @GPery, thanks for taking the time to improve Python!

As @tirkarthi mentionned, #11781 is already open to add this functionnality thought.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FortStatement

FortStatement commented Jun 11, 2019

Copy link
Copy Markdown
Author

#11781 adds a key parameter, which I believe is inferior. It's a specific case of a comparison callback, and in my use case would force me to create another class to serve as a comparator for my objects, at which point I might as well wrap them and add lt.

Furthermore, I believe this is more in-line with similar standard functions in other languages such as C++ (std::sort), Java (PriorityQueue) or Rust (slice.sort_by).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asvetlov

asvetlov commented Jun 11, 2019

Copy link
Copy Markdown
Contributor

key is more in line with Python list.sort() and sorted().
Use cmp_to_key() for handling __lt__ case.

NeilGirdhar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@remilapeyre

remilapeyre commented Jun 11, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

@GPery, both are actually two ways to describe the same behavior, do you know functools.cmp_to_key (https://docs.python.org/3/library/functools.html#functools.cmp_to_key) to go from one to the other?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FortStatement

FortStatement commented Jun 11, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

You're right, both cases are about the same.
The key extraction syntax still seems less intuitive to me than a compare function. Having been adopted by so many other languages, I think it should be the preferred behaviour, despite not matching list (after all, it's a separate library, even though it's builtin).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FortStatement

FortStatement commented Jun 11, 2019

Copy link
Copy Markdown
Author

I feel it might be relevant to note that this isn't reverting to the old-style (C-style) comparison.
It's a boolean comparison, parallel to how __lt__ behaves.
Not the old style, but a different alternative to it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asvetlov

asvetlov commented Jun 11, 2019

Copy link
Copy Markdown
Contributor

list sort uses key because this is much faster than old cmp method (very similar to your current proposal).
The reason is: key is calculated only once per item but cmp is called for every compared pair.
This is doesn't matter for fast compiled languages like C++ or Rust but in Python the python function call is relatively expensive.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FortStatement

FortStatement commented Jun 11, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

"every compared pair" is also (up to) once per item.
If we're on the subject of function calls, the key flow calls the key function, then the result's __lt__ function per item.
The compare_function flow only calls the callback.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jdemeyer

jdemeyer commented Jun 11, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

The reason is: key is calculated only once per item but cmp is called for every compared pair.

What's your point exactly? Even with custom keys, you still need to compare every compared pair with __lt__.

(Note: I do think that key is the way to go but only for compatibility with sorted(), not for performance).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asvetlov

asvetlov commented Jun 11, 2019

Copy link
Copy Markdown
Contributor

@jdemeyer IIRC at least for list sorting the point is that python-provided comparison function is usually much slower than built-in compare methods for int, str or tuple.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jdemeyer

jdemeyer commented Jun 11, 2019

Copy link
Copy Markdown
Contributor

at least for list sorting the point is that python-provided comparison function is usually much slower than built-in compare methods for int, str or tuple.

OK, so the performance argument becomes "in certain use cases, using key would be faster than a Python 2 style cmp". Fair enough. But there are certainly also uses cases (arguably less common) where the opposite is true, for example when using cmp_to_key.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FortStatement

FortStatement commented Jun 11, 2019

Copy link
Copy Markdown
Author

@asvetlov I'm not sure what were the original concerns, but I checked this PR and the key PR (#11781) with timeit, compare_function is extremely slightly, yet consistently, faster.

timeit -s "from bisect import bisect
                          class C:
                           def __init__(self, n):
                            self.n = n
                          data = [C(n) for n in range(1_000_000)]
                          cmp = C(25)" "bisect(data, cmp, key=lambda x: x.n)"
50000 loops, best of 5: 6.93 usec per loop
timeit -s "from bisect import bisect
                          class C:
                           def __init__(self, n):
                            self.n = n
                          data = [C(n) for n in range(1_000_000)]
                          cmp = C(25)" "bisect(data, cmp, compare_function=lambda a,b: a.n < b.n)"
50000 loops, best of 5: 6.79 usec per loop

I think this is a rather fair comparison.
Anyway, the real issue is syntax compatibility, which I think is subjective.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@remilapeyre

remilapeyre commented Jun 11, 2019

Copy link
Copy Markdown
Contributor

I just tried and had the opposite result, both with and without pydebug, I wonder what exactly might be doing that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rhettinger rhettinger self-assigned this Jun 11, 2019
@rhettinger rhettinger closed this Jun 11, 2019
@cirosantilli

cirosantilli commented Sep 3, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Hi there, why was this one closed? Was it superseded/WONTFIXED? This addition would be amazing, I keep hitting it. Let me know if there's any way I can help. Thanks to all.

ggorlen and lesshaste reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhettinger rhettinger Awaiting requested review from rhettinger

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@rhettinger rhettinger

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@FortStatement @asvetlov @remilapeyre @jdemeyer @cirosantilli @rhettinger @the-knights-who-say-ni @bedevere-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.