A* Search with Inconsistent Heuristics

[bbockman]

Summary of changes:

---main changes----

1. Created AStarInconsistentShortestPath class in the shortestpath package

This class extends AStarShortestPath and provides an optimization for inconsistent heuristics using the BPMX(1) style approach outlined in Inconsistent Heuristics in Theory and Practice. I have contacted the authors and they are completely fine with this being used in open source.

2. Modification to AStarShortestPath class

I simply changed some of the methods to protected to enable inheritance for the new class, and added javadoc comments when necessary to these newly protected methods.

3. Modifications to AStarShortestPath and BidirectionalAStarShortestPath

There was a bug in the current A* implementations that was preventing them from woking with arbitrary inconsistent heuristics. When vertices were moved from the closed list back to the open list, the vertexToHeapNodeMap was not being updated with the new AddressableHeap.Handle. I added a single update line to each of these files to correct this.

---test changes----

1. Created new AStarInconsistentShortestPathTest class

This is a copy paste of the AStarShortestPathTest class with updates to the constructor names so that it works with the new class.

2. Created new ALTInconsistentHeuristic class

This class is based on the ALTAdmissibleHeuristic class, and is meant for test purposes, but could be put into main aswell if deemed useful. It differs from ALTAdmissibleHeuristic in that instead of iterating over all the landmarks to find the maximum heuristic, it randomly selects a single heuristic for a given source node. This ensures admissiblity but not consistency. With any reasonable size graph it is extremely unlikely that this will produce a consisntent heuristic, hence it is used for testing with inconsistent heuristics.

3. Modifications to DijkstraShortestPathPerformanceTest class

I simply added the new AStarInconsistentShortestPath class to the set of tests in this file. I also added *ALTInc tests for the previous AStar implementations. These *ALTInc tests use getPath with the new ALTInconsistentHeuristic to test performance with inconsistent heuristics. This is mainly for comparison with the new AStarInconsistentShortestPath class.

Test Results:

---- Standard Tests -----

mvn -Dtest=AStarInconsistentShortestPath test
New class unit test ran successfully.

mvn install
Completed successfully.

"uses unchecked or unsafe operations"
Warnings produced from files unrelated to this PR.

"[WARNING] The following patterns were never triggered in this artifact inclusion filter o 'org.jgrapht:jgrapht-ext:*:combined'"
Produced, which is also completely unrelated to this change.

"uses deprecated API"
Warning produced by files unrelated to this PR. Screenshots of all of these are attached.

---- Performance Tests ----

DjikstraShortestPathPerformanceTest ran, comparision with previous A* implementations for both directed and undirected graphs attached. The state of the DjikstraShortestPathPerformanceTest class file in the PR reflects the remaining settings used for tests (all default/unchanged).

This AStarInconsistent class implementation appears to outperform the standard A* implementation on all test cases other than with no heuristic provided, which isn't a practical case. I think the performance results really understate its utility however, since the inconsistent heuristic generator used is made for arbitrary graphs and it isn't particularly good. This BPMX style approach really shines when used with quality inconsistent heurstics on graphs where they are appropriate. Instead of devising tests for this, I would like to defer to the following paper Inconsistent Heuristics in Theory and Practice which I believe gives a more thorough analysis than I would be able to within a reasonable amount of time.

Directed Graphs


Undirected Graphs

Issue: #808 A* Search with Inconsistent Heuristics.

• I read and understood https://github.com/jgrapht/jgrapht/wiki/Become-a-Contributor
• I read and understood https://github.com/jgrapht/jgrapht/wiki/How-to-make-your-first-%28code%29-contribution
• I added unit tests
• I added documentation
• I followed the Coding and Style Conventions
• I have not modified HISTORY.md or CONTRIBUTORS.md
• I ensured that the git commit message is a good one

[bbockman]

Pushes simply to amend the final commit comment to be more inline with standards.

[jkinable]

I went through this PR. Before we can consider this PR for a potential integration in jgrapht, there are a number of issues that need to be addressed.

1. The code assumes that the user is familiar with consistent/inconsistent heuristics. This is however not the case in practice. A description (both intuitive description as well as mathematical description would be necessary). The description should be self-contained such that the reader does not need to read the paper in order to understand the differences.
2. This PR introduces a large amount of code duplication. Think about how a consistent A* heuristic differs from a inconsistent implementation? Can you simply introduce an optional parameter which toggles consistent/inconsistent behavior? Or can you simply add a new AStarAdmissibleHeuristic?
3. Avoid vague language, e.g.: An admissible and probably inconsistent heuristic for the A* algorithm. What do you mean by 'probably inconsistent'? The user does not understand this. The user should not need to look at the code to understand what happens. Similar: A* with likely inconsistent heuristic. I'm having some trouble interpreting your computational results, but from the looks of it I'm not so convinced that the impact is visible from these benchmarks.

[bbockman]

Replying to #811 (comment) by @jkinable

Thanks for taking the time to look over this! I want to make sure I understand your concerns before I get started on updates.

For the shortest path algorithm itself, it sounds like you would prefer to condense both the standard A* implementation and the inconsistent heuristic optimization into a single class, and off-load the inconsistent heuristic specific calculations onto to the AStarAdmissibleHeuristic when appropriate.

I think this sounds like a great approach and is very doable. Let me know if I'm misunderstanding anything.

I believe I can create a sub-interface, AStarInconsistentHeuristic, extending AStarAdmissibleHeuristic; then I can use a flag based on the run-time type of the heuristic the user passes in to decide whether to run the extra update steps or not. This prevents any interface changes for existing code, and gives the user full control over whether to use the optimization or not. I'll put the methods for these update steps into the new interface. This should require minimal code changes to the current AStarShortestPath implementation, and should have no effect on the algorithm when using consistent heuristics.

For testing I can recreate the ALTInconsistentHeuristic class to implement the new AStarInconsistentHeuristic interface and extend ALTAdmissibleHeuristic. This change along with the previous item should remove virtually all duplicate code from both test and main.

As for the descriptions I can certainly add more detail and specifics, this shouldn't be a problem.
Lastly I can gather additional performance data that more thoroughly and clearly highlights the advantages of this optimization. Let me know if you have any specific tests in mind that will better help you decide on the utility of this update.

Thanks again for your time!

[bbockman]

Replying to #811 (comment) by @jkinable
Replying to #811 (comment) by @bbockman
Replying to #808 (comment) by @d-michail

So I went ahead and implemented the changes mentioned in my previous post. I believe this removed nearly all of the duplicate code, and should leave the original class unchanged (except for the bug fixes) when a client uses a standard heuristic. The optimization is performed only when the user passes in a heuristic that is of the newly created interface type AStarInconsistentHeuristic.

I have attached a more thorough run-time analysis for this optimization, it utilizes an interleaved inconsistent heuristic which does a much better job of demonstrating the usefulness of the algorithm.

A* - Interleaved, no BPMX is the standard A* algorithm while using this inconsistent heuristic and it is clearly asymptotically deficient due to excessive node expansions.

A* - Consistent is the standard A* algorithm on the same graph but using a consistent heuristic, this is much better since there aren't repeated node expansions, but still inferior to the the inconsistent heuristic for this class of graph.

A* - Interleaved, BPMX(1) this is the current inconsistent optimization implemented in this pull request which clearly performs the best on this particular class of graph.

A* - Interleaved, BPMX(inf) this is simply a reference for an alternative BPMX algorithm that looks ahead further when updating heuristics, this is more complex and again for this particular class of graph appears to have no advantage.

Lastly for the documentation, I did make some updates, however since I ended up removing most of the new classes and code, I didn't make many. Please let me know if the descriptions still are not clear enough and I can make further updates.

Thanks again to anyone that takes the time to review this, and I hope it is of some use to the project!

[bbockman]

Made a few more updates to javadoc comments.

[jsichi]

The current PR includes some changes to CONTRIBUTORS.md, HISTORY.md, and BiconnectivityInspector.java which shouldn't be part of it.

[bbockman]

Yes I merged changes from master before making updates. I did not realize github would show these as part of the pull request. I read pointing the PR to another branch and back would reset it, but that did not seem to work. At the end of the day these are changes that are in fact in master, so I’m not sure what the best remedy is.

[jsichi]

You either have to figure out how to rebase it correctly, or else create a completely new PR with the correct changes. If you go with the latter route, leave this one open and reference it, since you already have a lot of detailed information such as the graphs in the comments. Then when we merge the new one, we'll close this one.

[bbockman]

Replying to #811 (comment) by @jsichi

I rebased the branch to remove the BiconnectivityInspector commits. I think the revision history should now be in the form you wanted. Let me know if you need further changes. Thanks a bunch for reviewing this PR!

[jsichi]

@jkinable can you take a look to see if your comments have been satisfactorily resolved?

[jkinable]

This new version of the code without all the duplication is significantly improved. Most of my comments pertain the documentation. JGraphT is a library used by a vast number of people across my different research areas. Currently the description of the Inconsistent heuristics is insufficient for such a general audience. As a result, this code increases the complexity of the library, while it is unlikely that anyone is going to use it. The AStarShortestPath class is the main class, i.e. the point of access for most people. The various options and usages should be clearly outlined in this main class, instead of referring to other classes. Currently, it is not clear:

• what an inconsistent heuristic is
• when or why to use it
• how to use it

In general we try to make the documentation of JGraphT self-contained.

[jkinable]

Please update the description of AStarShortestPath: define admissible and inconsistent. You can borrow the description from wikipedia (or any other suitable source): https://en.wikipedia.org/wiki/Consistent_heuristic (just note that the notation used in wikipedia is quite awful).

Also, when possible, specify specific cases where a user might want to prefer a consistent over inconsistent heuristic and vice versa. The average user won't know the difference between the two. E.g. are there clear cases where an inconsistent heuristic will outperform a consistent heuristic and vice versa?
I would also specify a 'default' heuristic, e.g. is there a preference for consistent or inconsistent heuristics? Would you use consistent or inconsistent by default?

[jkinable]

I have no idea what a BPMX algorithm is. Please define this abreviation, and add a proper reference.

[jkinable]

This wording is confusing since your new AStarInconsistentHeuristic interface is also an AStarAdmissibleheuristic. Please rephrase. I'm not entirely sure what you intent to say.

[jkinable]

Correct the dates in your headers please. 2019-2019

[jkinable]

Please define:

• inconsistent
• BPMX style

Add proper references to relevant papers.

Is there some trivial example of an inconsistent heuristic suitable for astar?

[jkinable]

Without documentation, the method description is too shallow. Most users would not be able to implement this interface as it is quite unclear what exactly is expected and what it is used for.

[jkinable]

inconsistent = admissibleHeuristic instanceof AstartInconsistentHeuristic

[bbockman]

Replying to #811 (review) by @jkinable

Thank you for the detailed and specific feedback. I updated the javadoc comments to provide more details and suggestions to the user, particularly on the points you highlighted. The new interface for inconsistent heuristics should have had default methods for the update steps, I fixed this which should make it much much easier for users to implement and use the optimization.

I hope this is in line with your expectations so I don't need to take up more of your time in review. Thanks again for the help.