Merge changes from 3b #13
Conversation
ENQUEUE returns a handle which must be passed to above operations to specify element to modify.
| @@ -122,7 +127,7 @@ | |||
|
|
|||
| (defun test-dequeue-and-peek (queue list) | |||
| (let ((counter (q:size queue))) | |||
| (dotimes (i (length list)) | |||
| (dolist (i (sort list '<)) | |||
There was a problem hiding this comment.
This mutates the input list which might cause confusion. Change list to (copy-list list).
| (loop with length = (length vector) | ||
| for parent from 0 below (truncate length 2) | ||
| for left = (+ (* parent 2) 1) | ||
| for right = (+ (* parent 2) 2) | ||
| do (assert (< (aref vector parent) (aref vector left)) () | ||
| do (assert (<= (aref vector parent) (aref vector left)) () |
There was a problem hiding this comment.
In which case can these two be equal? Objects with equal priorities, or something?
There was a problem hiding this comment.
yeah, judging by the commit comment and the added test, that seems to be the intent.
| "VERIFY-HEAP-PROPERTY: Invalid left child: ~D -> ~D" | ||
| (aref vector parent) (aref vector left)) | ||
| when (oddp length) | ||
| do (assert (< (aref vector parent) (aref vector right)) () | ||
| do (assert (<= (aref vector parent) (aref vector right)) () |
| (asdf:defsystem #:damn-fast-updatable-priority-queue | ||
| :description "A heap-based priority queue with delete and adjust-priority whose first and foremost priority is speed." | ||
| :author "Michał \"phoe\" Herda <[email protected]>" | ||
| :license "MIT" | ||
| :version "0.0.2" | ||
| :serial t | ||
| :depends-on (#:alexandria) | ||
| :components ((:file "src")) | ||
| :in-order-to ((test-op (load-op #:damn-fast-updatable-priority-queue/test))) | ||
| :perform (test-op (o c) (symbol-call "DAMN-FAST-UPDATABLE-PRIORITY-QUEUE/TEST" "RUN"))) | ||
|
|
||
| (asdf:defsystem #:damn-fast-updatable-priority-queue/test | ||
| :description "Tests for Damn Fast Priority Queue" | ||
| :author "Michał \"phoe\" Herda <[email protected]>" | ||
| :license "MIT" | ||
| :version "0.0.2" | ||
| :serial t | ||
| :depends-on (#:alexandria #:damn-fast-updatable-priority-queue) | ||
| :components ((:file "test"))) |
There was a problem hiding this comment.
Modify to take "updatable" into account, also add @3b as a co-author.
| #+real-damn-fast-priority-queue | ||
| ;; Good luck. | ||
| `(optimize (speed 3) (debug 0) (safety 0) (space 0) (compilation-speed 0)) | ||
| #-real-damn-fast-priority-queue | ||
| `(optimize (speed 3)))) |
There was a problem hiding this comment.
Use a separate conditional, real-damn-fast-updatable-priority-queue.
| (perform-test queue (nreverse (a:iota length))) | ||
| (dotimes (i 100) | ||
| (perform-test queue (a:shuffle (a:iota length)))))) | ||
| (perform-error-test) |
There was a problem hiding this comment.
We seem to be missing the shuffle test here, the one that was added in standard DFPQ tests.
| @@ -193,6 +194,18 @@ | |||
| :peek-fn (lambda (q) (damn-fast-priority-queue:peek q)) | |||
| :pop-fn (lambda (q) (damn-fast-priority-queue:dequeue q)))) | |||
|
|
|||
| (defun test-damn-fast-updatable-priority-queue (vector-name vector) | |||
There was a problem hiding this comment.
Update the docs with the new benchmark results.
|
Been a while since i looked at this stuff, but i think merging it should be OK. I don't see any important looking changes in my local copy, just debugging noise. will try to take a closer look at it at some point though. |
|
is possible with this or with any damn-fast-*-priority-queue to dequeue and also get the priority number? |
|
It doesn't look like it, though that does sound like something that could be useful to have. |
|
I think it could work. We only depend on FOUNDP to be true right now, so
changing it to a generalized boolean to convey more information would be
a backwards compatible change.
…On 18.12.2021 01:16, 3b wrote:
It doesn't look like it, though that does sound like something that
could be useful to have.
Maybe the "found" value of |dequeue| and |peek| could be changed to
return priority in place of |T|? (at least for the updatable variant,
since it hadn't been merged yet. not sure if that would be too big a
change for the existing ones or not.)
—
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADSZHKVR3VXDM6OCDZKMQKLURPHETANCNFSM5JY2N7WA>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
@3b Will you mind if I review and merge your changes back into my branch? They seem backwards-compatible and should need just a bit of documentation.