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

feat: added Astar Search Algorithm in Graphs #1739

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mathangpeddi
Copy link

@mathangpeddi mathangpeddi commented Oct 20, 2024

Open in Gitpod know more

Describe your change:

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new JavaScript files are placed inside an existing directory.
  • All filenames should use the UpperCamelCase (PascalCase) style. There should be no spaces in filenames.
    Example:UserProfile.js is allowed but userprofile.js,Userprofile.js,user-Profile.js,userProfile.js are not
  • All new algorithms have a URL in their comments that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2024

Codecov Report

Attention: Patch coverage is 0% with 207 lines in your changes missing coverage. Please review.

Project coverage is 83.88%. Comparing base (55ff0ad) to head (6142d3d).

Files with missing lines Patch % Lines
Graphs/Astar.js 0.00% 207 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1739      +/-   ##
==========================================
- Coverage   84.76%   83.88%   -0.88%     
==========================================
  Files         378      379       +1     
  Lines       19738    19945     +207     
  Branches     2957     2958       +1     
==========================================
  Hits        16731    16731              
- Misses       3007     3214     +207     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mathangpeddi mathangpeddi changed the title Added Astar Search Algorithm feat: added Astar Search Algorithm in Graphs Oct 20, 2024
@mathangpeddi
Copy link
Author

@appgurueu @raklaptudirm
Can you please check this and let me know if this can be added to Graph Algorithms?

Graphs/Astar.js Outdated
@@ -0,0 +1,107 @@
/**
* Author: Mathang Peddi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use JSDoc annotations like @author.

Copy link
Author

Choose a reason for hiding this comment

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

Have changed it to @author: Mathang Peddi
Do I need to make any other change here?

Graphs/Astar.js Outdated
@@ -0,0 +1,107 @@
/**
* Author: Mathang Peddi
* A* Search Algorithm implementation in JavaScript
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* A* Search Algorithm implementation in JavaScript

Copy link
Author

Choose a reason for hiding this comment

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

Removed this line.

Graphs/Astar.js Outdated
* It uses graph data structure.
*/

function createGraph(V, E) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is unnecessary. Just take the graph in adjacency list representation. There is also no need to restrict this to undirected graphs.

Copy link
Author

Choose a reason for hiding this comment

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

Removed this function, created an adjacency list and directly passed it to the function.

Graphs/Astar.js Outdated
}

// Heuristic function to estimate the cost to reach the goal
// You can modify this based on your specific problem, for now, we're using Manhattan distance
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a useful manhattan distance. This is just the absolute distance between vertex IDs. Instead you want some kind of associated "points" (say, in 2d or 3d space) for which you compute distances.

In any case, the heuristic function should probably be a (mandatory) parameter.

Copy link
Author

Choose a reason for hiding this comment

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

I have made the changes, I am using Euclidian Distance here, do I need to make any other change here?

Graphs/Astar.js Outdated
return Math.abs(a - b)
}

function aStar(graph, V, src, target) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, just take the graph in adjacency list representation. V is then redundant (and also conflicts with our naming scheme).

Copy link
Author

Choose a reason for hiding this comment

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

Changed it.

Graphs/Astar.js Outdated
}
}

return [] // Return empty path if there's no path to the target
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be null instead.

Copy link
Author

Choose a reason for hiding this comment

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

Changed.

Graphs/Astar.js Outdated

function aStar(graph, V, src, target) {
const openSet = new Set([src]) // Nodes to explore
const cameFrom = Array(V).fill(-1) // Keep track of path
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use null instead of -1, it's the idiomatic value to use in JS for absence of a value.

Copy link
Author

Choose a reason for hiding this comment

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

Changed.

Graphs/Astar.js Outdated
openSet.delete(current)

// Explore neighbors
for (let i = 0; i < graph[current].length; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just do for (const [neighbor, weight] of graph[current])

Copy link
Author

Choose a reason for hiding this comment

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

Changed.

Graphs/Astar.js Outdated

module.exports = { createGraph, aStar }

// const V = 9
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be a test instead.

Copy link
Author

Choose a reason for hiding this comment

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

Removed this part, do you want me to add a Astar.test.js file as a part of Unit Testing?

Graphs/Astar.js Outdated
return [] // Return empty path if there's no path to the target
}

module.exports = { createGraph, aStar }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
module.exports = { createGraph, aStar }
export { aStar }

Copy link
Author

Choose a reason for hiding this comment

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

Changed.

@mathangpeddi
Copy link
Author

@appgurueu Can you check if all the changes are proper?

}

// Priority Queue (Min-Heap) implementation
class PriorityQueue {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please import this (we have a priority queue implementation in this repo) rather than reimplementing it.

}
}

function aStar(graph, src, target, points) {
Copy link
Collaborator

@appgurueu appgurueu Oct 23, 2024

Choose a reason for hiding this comment

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

Where are these parameters documented? Also why is the heuristic function not a parameter (which may default to a simple euclidean heuristic)?

return path.reverse()
}

// Explore neighbors using destructuring for cleaner code
Copy link
Collaborator

Choose a reason for hiding this comment

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

the "using destructuring for cleaner code part" is obvious

return null // Return null if there's no path to the target
}

// Define the graph as an adjacency list
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a proper test case.

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.

3 participants