Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-133530: Improve heapq documenation#133531
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"Improve" here is in the eye of the beholder. Please be more conservative and cautious when proposing changes, and split up changes into different PRs. This contains 3 distinct changes: introducing the image, introducing min-/max-heap terminology, and removing quite a bit of prose.
I also don't know if Raymond has yet had a chance to review the change to introduce the max heap function, so this should potentially wait on that happening first.
A
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can this be an SVG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a reason?
I used png since that is what hashlibs diagram uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
cpython/Doc/library$ find . -name '*.png'./pathlib-inheritance.png./tk_msg.png./tulip_coro.png./hashlib-blake2-tree.png./turtle-star.png./kde_example.pngcpython/Doc/library$ find . -name '*.svg'./pathlib-inheritance.svg| Min-heaps are arrays for which ``a[k] <= a[2*k+1]`` and ``a[k] <= a[2*k+2]`` for | ||
| all*k*, counting elements from 0. For the sake of comparison, non-existing | ||
| elements are considered to be infinite. The interesting property of a heap is | ||
| that ``a[0]`` is always its smallest element. | ||
| that ``a[0]`` is always its smallest element. Max-heaps are the reverse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it's fine to default to "heap" being a "min-heap", and then introduce "max-heap" afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes that is what is done elsewhere.
I changed it here for clarity.
| heap. So, a heap is a good structure for implementing schedulers (this is what | ||
| I used for my MIDI sequencer :-). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's a shame to 'sterilise' things by removing lines like this. If you feel strongly, perhaps rework to keep the example but remove the perpendicular pronoun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See#62480 for arguments for removing personal notes. Do we need an even more specific example, IMO the general example is fine. Again, this is formal, technical documentation — not a blog post.
Uh oh!
There was an error while loading.Please reload this page.
| Heaps are also very useful in big disk sorts.A | ||
| big sort implies producing "runs" (pre-sorted sequences, whose size is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think this needs to change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is more formal? I thought that is what documentation should be…?
| heap completely vanishes, you switch heaps and start a new run. Clever and | ||
| quite effective! | ||
| In a word, heaps are useful memory structures to know. I use them in a few | ||
| applications, and I think it is good to keep a 'heap' module around. :-) | ||
| .. rubric:: Footnotes | ||
| .. [#] The disk balancing algorithms which are current, nowadays, are more annoying | ||
| than clever, and this is a consequence of the seeking capabilities of the disks. | ||
| On devices which cannot seek, like big tape drives, the story was quite | ||
| different, and one had to be very clever to ensure (far in advance) that each | ||
| tape movement will be the most effective possible (that is, will best | ||
| participate at "progressing" the merge). Some tapes were even able to read | ||
| backwards, and this was also used to avoid the rewinding time. Believe me, real | ||
| good tape sorts were quite spectacular to watch! From all times, sorting has | ||
| always been a Great Art! :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Likewise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is there to keep there? It is a largely a personal note and if I were to strip it to the core information it is irrelevant anyway.
Do you not want formal documentation?
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I will split this for Adams convenience. |
Uh oh!
There was an error while loading.Please reload this page.
Removes a lot of unnecessary (and personal) parts. Also differentiate between heaps more, reference max-heap instead of a reverse list, and replace binary tree with a graphic I generated.
Some other thoughts:
Currently we use a pretty long sports analogy and use gender references, maybe a more neutral and technical description instead?
Add some headers for min/max heaps in the introduction for easier reference?
heapqdocumentation #133530📚 Documentation preview 📚:https://cpython-previews--133531.org.readthedocs.build/en/133531/library/heapq.html