I recently asked this question on the correctness of my Java implementation for the nearest neighbour algorithm for the travelling salesman problem. The user JayC667 gave the following critique:
As I have stated in my comments, I see one basic problem with the algorithm itself:
- It will NOT properly permute the towns, but always work in sequence (A-B-C-D-A-B-C-D, start anywhere and take 4)
As you can see, your algorithm is severely dependent on the sequence of input/towns.
I have rewritten my code as follows:
/*
* Returns the shortest tour found by exercising the NN algorithm
* from each possible starting city in table.
* table[i][j] == table[j][i] gives the cost of travel between City i and City j.
*/
public static int[] tspnn(double[][] table) {
int numberOfVertices = table.length;
int[] shortestHamiltonianCycle = new int[numberOfVertices];
double lowestHamiltonianCycleCost = Double.POSITIVE_INFINITY;
// consider each vertex as a starting vertex
for(int startingVertex = 0; startingVertex < numberOfVertices; startingVertex++) {
// initialize all vertices as unvisited
boolean[] visitedVertices = new boolean[numberOfVertices];
int[] currentShortestHamiltonianCycle = new int[numberOfVertices];
/*
* Always initialize the first element of currentShortestHamiltonianCycle
* to be the starting vertex.
*/
currentShortestHamiltonianCycle[0] = startingVertex;
// always initialize the starting vertex as having been visited.
visitedVertices[startingVertex] = true;
double currentLowestCostHamiltonianCycle = 0;
int currentNearestVertex = startingVertex;
/*
* The first element of currentShortestHamiltonianCycle will always
* be the starting vertex, so we can start our loop at index 1.
* We progressively populate currentShortestHamiltonianCycle with
* the nearest vertex.
*/
for(int i = 1; i < currentShortestHamiltonianCycle.length; i++) {
double currentLowestNeighbourCost = Double.POSITIVE_INFINITY;
/*
* Find the nearest vertex to currentShortestHamiltonianCycle[i].
* To do this, we must check all vertices.
*/
for(int checkVertex = 0; checkVertex < numberOfVertices; checkVertex++) {
// only if the vertex is not part of currentShortestHamiltonianCycle
if(visitedVertices[checkVertex] == false) {
double currentNeighbourCost = table[currentShortestHamiltonianCycle[i - 1]][checkVertex];
if(currentNeighbourCost < currentLowestNeighbourCost) {
currentLowestNeighbourCost = currentNeighbourCost;
currentNearestVertex = checkVertex;
}
}
}
/*
* At this point, currentNearestVertex must be the nearest vertex to
* the vertex currentShortestHamiltonianCycle[i - 1]
*/
// currentNearestVertex must be the nearest, so mark it as visited
visitedVertices[currentNearestVertex] = true;
// add currentNearestVertex to the current shortest Hamiltonian cycle
currentShortestHamiltonianCycle[i] = currentNearestVertex;
/*
* add the cost of currentNearestVertex to the
* running total cost for the current Hamiltonian cycle
*/
currentLowestCostHamiltonianCycle += currentLowestNeighbourCost;
}
/*
* At this point, currentShortestHamiltonianCycle must be the shortest
* Hamiltonian cycle for this starting point. If it's also the shortest
* Hamiltonian cycle over all starting points so far, then set it as
* the overall shortest Hamiltonian cycle.
*/
if(currentLowestCostHamiltonianCycle < lowestHamiltonianCycleCost) {
shortestHamiltonianCycle = currentShortestHamiltonianCycle;
lowestHamiltonianCycleCost = currentLowestCostHamiltonianCycle;
}
}
return shortestHamiltonianCycle;
}
And I have used JayC667's tester with this new code as follows:
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
public class NNtester {
static public final int NUMBER_OF_TEST_RUNS = 4;
static public final boolean GENERATE_SIMPLE_TOWNS = true;
static public final int NUMBER_OF_COMPLEX_TOWNS = 10;
static public final int DISTANCE_RANGE_OF_COMPLEX_TOWNS = 100;
static private class Town {
public final String Name;
public final int X;
public final int Y;
public Town(final String pName, final int pX, final int pY) {
Name = pName;
X = pX;
Y = pY;
}
public double getDistanceTo(final Town pOther) {
final int dx = pOther.X - X;
final int dy = pOther.Y - Y;
return Math.sqrt(Math.abs(dx * dx + dy * dy));
}
@Override public int hashCode() { // not really needed here
final int prime = 31;
int result = 1;
result = prime * result + X;
result = prime * result + Y;
return result;
}
@Override public boolean equals(final Object obj) {
if (this == obj) return true;
if (obj == null) return false;
if (getClass() != obj.getClass()) return false;
final Town other = (Town) obj;
if (X != other.X) return false;
if (Y != other.Y) return false;
return true;
}
@Override public String toString() {
return Name + " (" + X + "/" + Y + ")";
}
}
static private double[][] generateDistanceTable(final ArrayList<Town> pTowns) {
final double[][] ret = new double[pTowns.size()][pTowns.size()];
for (int outerIndex = 0; outerIndex < pTowns.size(); outerIndex++) {
final Town outerTown = pTowns.get(outerIndex);
for (int innerIndex = 0; innerIndex < pTowns.size(); innerIndex++) {
final Town innerTown = pTowns.get(innerIndex);
final double distance = outerTown.getDistanceTo(innerTown);
ret[outerIndex][innerIndex] = distance;
}
}
return ret;
}
static private ArrayList<Town> generateTowns_simple() {
final Town a = new Town("A", 0, 0);
final Town b = new Town("B", 1, 0);
final Town c = new Town("C", 2, 0);
final Town d = new Town("D", 3, 0);
return new ArrayList<>(Arrays.asList(a, b, c, d));
}
static private ArrayList<Town> generateTowns_complex() {
final ArrayList<Town> allTowns = new ArrayList<>();
for (int i = 0; i < NUMBER_OF_COMPLEX_TOWNS; i++) {
final int randomX = (int) (Math.random() * DISTANCE_RANGE_OF_COMPLEX_TOWNS);
final int randomY = (int) (Math.random() * DISTANCE_RANGE_OF_COMPLEX_TOWNS);
final Town t = new Town("Town-" + (i + 1), randomX, randomY);
if (allTowns.contains(t)) { // do not allow different towns at same location!
System.out.println("Towns colliding at " + t);
--i;
} else {
allTowns.add(t);
}
}
return allTowns;
}
static private ArrayList<Town> generateTowns() {
if (GENERATE_SIMPLE_TOWNS) return generateTowns_simple();
else return generateTowns_complex();
}
static private void printTowns(final ArrayList<Town> pTowns, final double[][] pDistances) {
System.out.println("Towns:");
for (final Town town : pTowns) {
System.out.println("\t" + town);
}
System.out.println("Distance Matrix:");
for (int y = 0; y < pDistances.length; y++) {
System.out.print("\t");
for (int x = 0; x < pDistances.length; x++) {
System.out.print(pDistances[y][x] + " (" + pTowns.get(y).Name + "-" + pTowns.get(x).Name + ")" + "\t");
}
System.out.println();
}
}
private static void testAlgorithm() {
final ArrayList<Town> towns = generateTowns();
for (int i = 0; i < NUMBER_OF_TEST_RUNS; i++) {
final double[][] distances = generateDistanceTable(towns);
printTowns(towns, distances);
{
final int[] path = tspnn(distances);
System.out.println("tspnn Path:");
for (int pathIndex = 0; pathIndex < path.length; pathIndex++) {
final Town t = towns.get(pathIndex);
System.out.println("\t" + t);
}
}
{
final ArrayList<Town> path = tspnn_simpleNN(towns);
System.out.println("tspnn_simpleNN Path:");
for (final Town t : path) {
System.out.println("\t" + t);
}
System.out.println("\n");
}
// prepare for for next run. We do this at the end of the loop so we can only print first config
Collections.shuffle(towns);
}
}
public static void main(final String[] args) {
testAlgorithm();
}
/*
* Returns the shortest tour found by exercising the NN algorithm
* from each possible starting city in table.
* table[i][j] == table[j][i] gives the cost of travel between City i and City j.
*/
public static int[] tspnn(double[][] table) {
int numberOfVertices = table.length;
int[] shortestHamiltonianCycle = new int[numberOfVertices];
double lowestHamiltonianCycleCost = Double.POSITIVE_INFINITY;
// consider each vertex as a starting vertex
for(int startingVertex = 0; startingVertex < numberOfVertices; startingVertex++) {
// initialize all vertices as unvisited
boolean[] visitedVertices = new boolean[numberOfVertices];
int[] currentShortestHamiltonianCycle = new int[numberOfVertices];
/*
* Always initialize the first element of currentShortestHamiltonianCycle
* to be the starting vertex.
*/
currentShortestHamiltonianCycle[0] = startingVertex;
// always initialize the starting vertex as having been visited.
visitedVertices[startingVertex] = true;
double currentLowestCostHamiltonianCycle = 0;
int currentNearestVertex = startingVertex;
/*
* The first element of currentShortestHamiltonianCycle will always
* be the starting vertex, so we can start our loop at index 1.
* We progressively populate currentShortestHamiltonianCycle with
* the nearest vertex.
*/
for(int i = 1; i < currentShortestHamiltonianCycle.length; i++) {
double currentLowestNeighbourCost = Double.POSITIVE_INFINITY;
/*
* Find the nearest vertex to currentShortestHamiltonianCycle[i].
* To do this, we must check all vertices.
*/
for(int checkVertex = 0; checkVertex < numberOfVertices; checkVertex++) {
// only if the vertex is not part of currentShortestHamiltonianCycle
if(visitedVertices[checkVertex] == false) {
double currentNeighbourCost = table[currentShortestHamiltonianCycle[i - 1]][checkVertex];
if(currentNeighbourCost < currentLowestNeighbourCost) {
currentLowestNeighbourCost = currentNeighbourCost;
currentNearestVertex = checkVertex;
}
}
}
/*
* At this point, currentNearestVertex must be the nearest vertex to
* the vertex currentShortestHamiltonianCycle[i - 1]
*/
// currentNearestVertex must be the nearest, so mark it as visited
visitedVertices[currentNearestVertex] = true;
// add currentNearestVertex to the current shortest Hamiltonian cycle
currentShortestHamiltonianCycle[i] = currentNearestVertex;
/*
* add the cost of currentNearestVertex to the
* running total cost for the current Hamiltonian cycle
*/
currentLowestCostHamiltonianCycle += currentLowestNeighbourCost;
}
/*
* At this point, currentShortestHamiltonianCycle must be the shortest
* Hamiltonian cycle for this starting point. If it's also the shortest
* Hamiltonian cycle over all starting points so far, then set it as
* the overall shortest Hamiltonian cycle.
*/
if(currentLowestCostHamiltonianCycle < lowestHamiltonianCycleCost) {
shortestHamiltonianCycle = currentShortestHamiltonianCycle;
lowestHamiltonianCycleCost = currentLowestCostHamiltonianCycle;
}
}
return shortestHamiltonianCycle;
}
/**
* Here come my basic implementations.
* They can be heavily (heavily!) improved, but are verbose and direct to show the logic behind them
*/
/**
* <p>example how to implement the NN solution th OO way</p>
* we could also implement
* <ul>
* <li>a recursive function</li>
* <li>or one with running counters</li>
* <li>or one with a real map/route objects, where further optimizations can take place</li>
* </ul>
*/
public static ArrayList<Town> tspnn_simpleNN(final ArrayList<Town> pTowns) {
ArrayList<Town> bestRoute = null;
double bestCosts = Double.MAX_VALUE;
for (final Town startingTown : pTowns) {
//setup
final ArrayList<Town> visitedTowns = new ArrayList<>(); // ArrayList because we need a stable index
final HashSet<Town> unvisitedTowns = new HashSet<>(pTowns); // all towns are available at start; we use HashSet because we need fast search; indexing plays not role here
// step 1
Town currentTown = startingTown;
visitedTowns.add(currentTown);
unvisitedTowns.remove(currentTown);
// steps 2-n
while (unvisitedTowns.size() > 0) {
// find nearest town
final Town nearestTown = findNearestTown(currentTown, unvisitedTowns);
if (nearestTown == null) throw new IllegalStateException("Something in the code is wrong...");
currentTown = nearestTown;
visitedTowns.add(currentTown);
unvisitedTowns.remove(currentTown);
}
// selection
final double cost = getCostsOfRoute(visitedTowns);
if (cost < bestCosts) {
bestCosts = cost;
bestRoute = visitedTowns;
}
}
return bestRoute;
}
static private Town findNearestTown(final Town pCurrentTown, final HashSet<Town> pSelectableTowns) {
double minDist = Double.MAX_VALUE;
Town minTown = null;
for (final Town checkTown : pSelectableTowns) {
final double dist = pCurrentTown.getDistanceTo(checkTown);
if (dist < minDist) {
minDist = dist;
minTown = checkTown;
}
}
return minTown;
}
static private double getCostsOfRoute(final ArrayList<Town> pTowns) {
double costs = 0;
for (int i = 1; i < pTowns.size(); i++) { // use pre-index
final Town t1 = pTowns.get(i - 1);
final Town t2 = pTowns.get(i);
final double cost = t1.getDistanceTo(t2);
costs += cost;
}
return costs;
}
}
Using this tester, I get the following output:
Towns:
A (0/0)
B (1/0)
C (2/0)
D (3/0)
Distance Matrix:
0.0 (A-A) 1.0 (A-B) 2.0 (A-C) 3.0 (A-D)
1.0 (B-A) 0.0 (B-B) 1.0 (B-C) 2.0 (B-D)
2.0 (C-A) 1.0 (C-B) 0.0 (C-C) 1.0 (C-D)
3.0 (D-A) 2.0 (D-B) 1.0 (D-C) 0.0 (D-D)
tspnn Path:
A (0/0)
B (1/0)
C (2/0)
D (3/0)
tspnn_simpleNN Path:
A (0/0)
B (1/0)
C (2/0)
D (3/0)
Towns:
A (0/0)
B (1/0)
D (3/0)
C (2/0)
Distance Matrix:
0.0 (A-A) 1.0 (A-B) 3.0 (A-D) 2.0 (A-C)
1.0 (B-A) 0.0 (B-B) 2.0 (B-D) 1.0 (B-C)
3.0 (D-A) 2.0 (D-B) 0.0 (D-D) 1.0 (D-C)
2.0 (C-A) 1.0 (C-B) 1.0 (C-D) 0.0 (C-C)
tspnn Path:
A (0/0)
B (1/0)
D (3/0)
C (2/0)
tspnn_simpleNN Path:
A (0/0)
B (1/0)
C (2/0)
D (3/0)
Towns:
C (2/0)
A (0/0)
D (3/0)
B (1/0)
Distance Matrix:
0.0 (C-C) 2.0 (C-A) 1.0 (C-D) 1.0 (C-B)
2.0 (A-C) 0.0 (A-A) 3.0 (A-D) 1.0 (A-B)
1.0 (D-C) 3.0 (D-A) 0.0 (D-D) 2.0 (D-B)
1.0 (B-C) 1.0 (B-A) 2.0 (B-D) 0.0 (B-B)
tspnn Path:
C (2/0)
A (0/0)
D (3/0)
B (1/0)
tspnn_simpleNN Path:
A (0/0)
B (1/0)
C (2/0)
D (3/0)
Towns:
B (1/0)
A (0/0)
C (2/0)
D (3/0)
Distance Matrix:
0.0 (B-B) 1.0 (B-A) 1.0 (B-C) 2.0 (B-D)
1.0 (A-B) 0.0 (A-A) 2.0 (A-C) 3.0 (A-D)
1.0 (C-B) 2.0 (C-A) 0.0 (C-C) 1.0 (C-D)
2.0 (D-B) 3.0 (D-A) 1.0 (D-C) 0.0 (D-D)
tspnn Path:
B (1/0)
A (0/0)
C (2/0)
D (3/0)
tspnn_simpleNN Path:
A (0/0)
B (1/0)
C (2/0)
D (3/0)
Based on the output of this tester, my new code seems to have the same problem. The first output for my code seems to be the same as JayC667's code's output:
tspnn Path:
A (0/0)
B (1/0)
C (2/0)
D (3/0)
tspnn_simpleNN Path:
A (0/0)
B (1/0)
C (2/0)
D (3/0)
However, my code seems to produce different output for the three subsequent cases:
2.
tspnn Path:
A (0/0)
B (1/0)
D (3/0)
C (2/0)
tspnn_simpleNN Path:
A (0/0)
B (1/0)
C (2/0)
D (3/0)
tspnn Path:
C (2/0)
A (0/0)
D (3/0)
B (1/0)
tspnn_simpleNN Path:
A (0/0)
B (1/0)
C (2/0)
D (3/0)
tspnn Path:
B (1/0)
A (0/0)
C (2/0)
D (3/0)
tspnn_simpleNN Path:
A (0/0)
B (1/0)
C (2/0)
D (3/0)
So clearly my implementation/method is producing different results compared to JayC667's tspnn_simpleNN method. And JayC667 has said that the problem seems to be that my implementation will not "properly permute the towns, but always work in sequence". The problem is that I don't really understand what this means, or how I've even built this bug into the implementation. I'm new to implementing algorithms, so it might be that I keep building this bug into my implementations of NN because I'm misunderstanding how it's supposed to work. So what exactly is this problem with the permutations? How is the implementation actually supposed to work? And how do I fix it?
