classification
Title: Small improvements in heapq (refactoring)
Type: enhancement Stage: resolved
Components: Versions: Python 3.8
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: amper, rhettinger
Priority: normal Keywords: patch

Created on 2018-07-24 15:06 by amper, last changed 2018-07-26 06:53 by rhettinger. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 8439 closed amper, 2018-07-24 15:08
Messages (3)
msg322307 - (view) Author: Alexander Marshalov (amper) * Date: 2018-07-24 15:06
I would like to make three small improvements to the "heapq" module.

1) The "nsmallest" function has the following code (a similar code exists in the "nlargest" function):

    # When n>=size, it's faster to use sorted()
    try:
        size = len(iterable)
    except (TypeError, AttributeError):
        pass
    else:
        if n >= size:
            return sorted(iterable, key=key)[:n]

   I think "[:n]" is redundant, because "iterable" contains no more than n elements.
   Therefore, that code can be rewritten as follows:
   
    # When n>=size, it's faster to use sorted()
    try:
        size = len(iterable)
    except (TypeError, AttributeError):
        pass
    else:
        if n >= size:
            return sorted(iterable, key=key)

2) It seems to me that the line:

    for i in reversed(range(n//2)):

   will be more optimal in this version:

    for i in range(n//2 + 1, -1, -1):


3) Top-level functions can be surrounded with two blank lines for greater compliance with PEP-8.
msg322310 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-07-24 15:40
Sorry, I don't consider any of these changes to be improvements and some make the code worse in some ways, overriding intentional coding decisions.
msg322404 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-07-26 06:53
Additional notes:  

* reversed() is preferred over the range(n-1, -1, -1) style both for clarity and speed.  If reversed is slower, then it would be mean that something is sly wrong with range.__reversed__ which should be able to iterate backwards as fast as range.__iter__ can go forwards (in part because they would use substantially the same code).

* Minor PEP 8 whitespace changes are generally not accepted. Usually, the code churn isn't worth it.

* [:n] is superfluous but it serves as a nice reminder of the definition of the function and will prevent a bug when I tweak the cut-over point from n >= size to some large fraction of the size (a change I'm currently considering).
History
Date User Action Args
2018-07-26 06:53:54rhettingersetmessages: + msg322404
2018-07-24 15:40:00rhettingersetstatus: open -> closed

nosy: + rhettinger
messages: + msg322310

resolution: rejected
stage: patch review -> resolved
2018-07-24 15:08:48ampersettitle: Small improvements in heapq (refatoring) -> Small improvements in heapq (refactoring)
2018-07-24 15:08:03ampersetkeywords: + patch
stage: patch review
pull_requests: + pull_request7965
2018-07-24 15:06:20ampercreate