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(collections): Fix edge cases for BinaryHeap #2525

Merged
merged 1 commit into from
Aug 14, 2022

Conversation

KyleJune
Copy link
Contributor

There were 2 issues and I fixed both. I found them while investigating issue #2522.

  1. push was using the wrong algorithm for finding the parent index. It would end up comparing and swapping with the wrong index in some cases. The first test case below pins the correct behavior. The test case passes for the heap npm module too.
  2. pop would swap with the wrong child if both were less than the parent value. It would always swap with the left instead of the one that is lowest.

Below are 2 quotes from the Binary Heap Wikipedia page.

Steps for insertion:

  1. Add the element to the bottom level of the heap at the leftmost open space.
  2. Compare the added element with its parent; if they are in the correct order, stop.
  3. If not, swap the element with its parent and return to the previous step.

Steps for extraction:

  1. Replace the root of the heap with the last element on the last level.
  2. Compare the new root with its children; if they are in the correct order, stop.
  3. If not, swap the element with one of its children and return to the previous step. (Swap with its smaller child in a min-heap and its larger child in a max-heap.)

Here are all 3 edge case tests I wrote for collections/BinaryHeap. They all pass now with the fixes I've made to push and pop.

Deno.test("[collections/BinaryHeap] edge case 1", () => {
  const minHeap = new BinaryHeap<number>(ascend);
  minHeap.push(4, 2, 8, 1, 10, 7, 3, 6, 5);
  assertEquals(minHeap.pop(), 1);
  minHeap.push(9);

  const expected = [2, 3, 4, 5, 6, 7, 8, 9, 10];
  assertEquals([...minHeap], expected);
});

Deno.test("[collections/BinaryHeap] edge case 2", () => {
  interface Point {
    x: number;
    y: number;
  }
  const minHeap = new BinaryHeap<Point>((a, b) => ascend(a.x, b.x));
  minHeap.push({ x: 0, y: 1 }, { x: 0, y: 2 }, { x: 0, y: 3 });

  const expected = [{ x: 0, y: 1 }, { x: 0, y: 3 }, { x: 0, y: 2 }];
  assertEquals([...minHeap], expected);
});

Deno.test("[collections/BinaryHeap] edge case 3", () => {
  interface Point {
    x: number;
    y: number;
  }
  const minHeap = new BinaryHeap<Point>((a, b) => ascend(a.x, b.x));
  minHeap.push(
    { x: 0, y: 1 },
    { x: 1, y: 2 },
    { x: 1, y: 3 },
    { x: 2, y: 4 },
    { x: 2, y: 5 },
    { x: 2, y: 6 },
    { x: 2, y: 7 },
  );

  const expected = [
    { x: 0, y: 1 },
    { x: 1, y: 2 },
    { x: 1, y: 3 },
    { x: 2, y: 5 },
    { x: 2, y: 4 },
    { x: 2, y: 6 },
    { x: 2, y: 7 },
  ];
  assertEquals([...minHeap], expected);
});

Here are what the 3 different heaps from these test cases look like before the values are removed for comparing their pop order.

Edge case 1 heap:
  Before pop:
               1
          2        3
      4     10   8   7
    6   5
  After pop:
               2
          4        3
      5     10   8   7
    6
  After push:
               2
          4        3
      5     10   8   7
    6   9


Edge case 2 heap:
      0,1
  0,2     0,3

Edge case 3 heap:
              0,1
      1,2             1,3
  2,4     2,5     2,6     2,7

Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!!!!

@kitsonk kitsonk merged commit 954c868 into denoland:main Aug 14, 2022
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.

2 participants