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

Break astar ties using heuristic value #5344

Closed
wants to merge 2 commits into from
Closed

Break astar ties using heuristic value #5344

wants to merge 2 commits into from

Conversation

lior8
Copy link
Contributor

@lior8 lior8 commented Feb 19, 2022

I changed A* tiebreaking to rely on the heuristic value of a node, and only then on the counter. This makes it significantly faster when dealing with searches where the path is "clear" to the algorithm, upwards of X10 faster than the current tiebreaker.

An alternative could be to allow the user to choose the tiebreaking via an additional optional parameter specifying if to tiebreak via heuristic value or not. Or even give the user the option to list a function which will recieve the current node, the target node, and will return a tuple specifying the tiebreaking value (while still using the counter to make sure no two elements are the same).

@dschult
Copy link
Member

dschult commented Jun 19, 2022

This looks like a nice improvement to me!
Could you add some words to the paragraph at line 16 (it is inside the doc_string)? That line makes it clear that we only return one path when there are more than one. That seems like a good place to describe how we break ties. Your description above is quite good so use something like that.

Thanks!

@dschult dschult added this to the networkx-3.0 milestone Jun 19, 2022
@lior8
Copy link
Contributor Author

lior8 commented Jun 19, 2022

I added a line that explains about the tiebreaking.
I would like to say that in general, I think the A* functions need a bit of an update in terms of variable names and terms used to be more consistent with heuristic search conventions. Will it be alright if I updated it, verify my implementation with the existing tests (and write more if there is a need), and open a new PR for it?

@dschult
Copy link
Member

dschult commented Jun 20, 2022

Can you give an example of something that you would change?
Or even just a pointer to what you mean by "consistent with heuristic search conventions". What conventions? By whose interpretation?
We don't have a lot of time to review changes to variable names, and prefer PRs like this one where it is pretty easy to see what is happening. OTOH if the changes are going to make the code significantly more readable and easier to figure out what it is actually doing, then that would definitely be worthwhile. The problem is that there is a lot of grey area between those extremes. :}

@lior8
Copy link
Contributor Author

lior8 commented Jun 29, 2022

There is an error we the current code. I will fix it and open a new PR once its done

@lior8 lior8 closed this Jun 29, 2022
@lior8 lior8 deleted the astar-heuristic-tiebreaking branch June 29, 2022 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants