Skip to content

[BUG]: method pop in PrimMST.js not working properly #1298

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

Closed
paulinegarelli opened this issue Feb 21, 2023 · 3 comments · Fixed by #1300
Closed

[BUG]: method pop in PrimMST.js not working properly #1298

paulinegarelli opened this issue Feb 21, 2023 · 3 comments · Fixed by #1300
Labels
bug Something isn't working

Comments

@paulinegarelli
Copy link
Contributor

Description

In Graphs/PrimMST.js, the method _shiftDown of class PriorityQueue does not give the correct output when a node must be shifted to a left-child leaf without a right-child "brother", which results in a wrong behavior of method pop.

Expected Behavior

An example of a tree that produces the bug is given by the following array of nodes denoted by their priority:
queue = [0, 1, 2]

ie

  1. root node of priority 0
  2. left leaf of priority 1
  3. right leaf of priority 0

If we call method pop on this queue, we expect to get the new tree:
queue = [1, 2]

ie

  1. root node of priority 1
  2. left leaf of priority 2

Actual Behavior

In the case described above, we will get as output a tree represented by:
queue = [2, 1]

ie

  1. root node of priority 2
  2. left leaf of priority 1

This violates the properties of the heap (we have 2 > 1 but a parent node should never have a bigger value than any of its children).

Steps to reproduce (if applicable)

  1. Add an export statement for class PriorityQueue in PrimMST.js

export { PriorityQueue }

  1. Add a test file PrimMST.test.js containing the following code
import { PriorityQueue} from '../PrimMST.js'

test('Bug in pop method', () => {
    // create queue
    const queue = new PriorityQueue()
    queue.push(0, 0)
    queue.push(1, 1)
    queue.push(2, 2)
    // create expected queue
    const expectedQueue = new PriorityQueue()
    expectedQueue.push(1, 1)
    expectedQueue.push(2, 2)
    // result from popping element from the queue
    queue.pop()
    expect(queue).toEqual(expectedQueue)
})
  1. Run the tests for PrimMST

npm test -- PrimMST.test.js

Additional information

This is due to the fact that in _shiftDown, the children's priorities are computed under a single try {} catch {} statement, thus the priority of the left child is set to Infinity if the right child does not exist.

@paulinegarelli paulinegarelli added the bug Something isn't working label Feb 21, 2023
@paulinegarelli
Copy link
Contributor Author

I am working on correcting the bug, refactoring the method _shiftDown for a clearer computation of nodes priorities, and adding tests for PrimMST and the pop method.

@appgurueu
Copy link
Collaborator

appgurueu commented Feb 21, 2023

I am working on correcting the bug, refactoring the method _shiftDown for a clearer computation of nodes priorities, and adding tests for PrimMST and the pop method.

The priority queue should not be a part of PrimMST, it should be its own algo (or rather, data structure) with its own implementation and tests and should only be imported and used by PrimMST.

Please have a look - it should be possible to merge the PQ used & implemented in PrimMST with our existing PQ implementation: https://github.com/TheAlgorithms/JavaScript/blob/master/Data-Structures/Heap/MinPriorityQueue.js

paulinegarelli added a commit to paulinegarelli/JavaScript that referenced this issue Feb 22, 2023
paulinegarelli added a commit to paulinegarelli/JavaScript that referenced this issue Feb 22, 2023
paulinegarelli added a commit to paulinegarelli/JavaScript that referenced this issue Feb 22, 2023
paulinegarelli added a commit to paulinegarelli/JavaScript that referenced this issue Feb 22, 2023
paulinegarelli added a commit to paulinegarelli/JavaScript that referenced this issue Feb 22, 2023
paulinegarelli added a commit to paulinegarelli/JavaScript that referenced this issue Feb 22, 2023
@paulinegarelli
Copy link
Contributor Author

I couldn't merge the two implementations because the PQ we use has keys in addition to the priority value to be able to track the nodes of the graph, which none of the already existing implementations have.
So I added this implementation as a new algorithm.

paulinegarelli added a commit to paulinegarelli/JavaScript that referenced this issue Feb 22, 2023
raklaptudirm pushed a commit that referenced this issue Feb 23, 2023
* ref: KeyPriorityQueue in separate  file #1298

* feat: add tests for KeyPriorityQueue #1298

* fix: _shiftDown refactored and corrected #1298

* fix: use KeyPriorityQueue in PrimMST #1298

* feat: add test for PrimMST #1298

* fix: format files #1298

* fix: minor coding style changes

* fix: use map for keys and priorities #1298
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants