Skip to content
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

Fix adding functions to CommandChainDispatcher with equal priority on Py 3 #1932

Merged
merged 2 commits into from Jun 12, 2012

Conversation

takluyver
Copy link
Member

Includes a test.

Closes gh-1916

@fperez
Copy link
Member

fperez commented Jun 12, 2012

Code looks clean, running test_pr now

@takluyver
Copy link
Member Author

Race you with the tests ;-)

@minrk
Copy link
Member

minrk commented Jun 12, 2012

This looks solid, but just for my own edification, why use operator.itemgetter(0) instead of lambda t: t[0]?

@takluyver
Copy link
Member Author

Test results for commit 2e28c7b merged into master
Platform: linux2

  • python2.7: OK (libraries not available: rpy2 tornado)
  • python3.1: OK (libraries not available: cython matplotlib numpy pymongo qt rpy2 wx wx.aui zmq)
  • python3.2: OK (libraries not available: cython pymongo rpy2 wx wx.aui)

Not available for testing: python2.6

@fperez
Copy link
Member

fperez commented Jun 12, 2012

Test results for commit 2e28c7b merged into master
Platform: linux2

  • python2.7: OK
  • python3.2: OK (libraries not available: cython matplotlib pymongo rpy2 wx wx.aui)

Not available for testing: python2.6, python3.1

@fperez
Copy link
Member

fperez commented Jun 12, 2012

I had the same thought as @minrk, decided not to really bring it up, but I also think the lambda in this case is cleaner than the rather awkward-looking itemgetter call. Order epsilon complaint, though :)

I'm OK with it either way, otherwise I think this can go in.

@takluyver
Copy link
Member Author

I understand that itemgetter is faster - not that it makes a jot of difference here, but just as general 'best practices'. On reflection, I think I'd actually prefer the lambda here as well. I'll commit that and then merge.

@minrk
Copy link
Member

minrk commented Jun 12, 2012

Quick test shows it is a good deal faster:

In [17]: import operator
In [18]: f = operator.itemgetter(0)
In [19]: g = lambda t: t[0]
In [20]: chain = [(i,'foo') for i in range(1000)]

In [21]: %timeit chain.sort(key=f)
10000 loops, best of 3: 163 us per loop

In [22]: %timeit chain.sort(key=g)
1000 loops, best of 3: 289 us per loop

So almost twice as fast. But that's still only saving 0.1ms on a thousand items (already sorted).

@takluyver
Copy link
Member Author

I guess it's the overhead of an repeatedly making an extra stack frame for the lambda - the operator module is compiled, so the sort can stay in C code.

takluyver added a commit that referenced this pull request Jun 12, 2012
Fix adding functions to CommandChainDispatcher with equal priority on Py 3
@takluyver takluyver merged commit b6137d2 into ipython:master Jun 12, 2012
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Fix adding functions to CommandChainDispatcher with equal priority on Py 3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

%paste doesn't work on py3
3 participants