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: solve TSP using branch and bound method #34

Merged
merged 27 commits into from
Aug 2, 2023
Merged

Feat: solve TSP using branch and bound method #34

merged 27 commits into from
Aug 2, 2023

Conversation

luanleonardo
Copy link
Collaborator

@luanleonardo luanleonardo commented Jul 22, 2023

Description

An exact Traveling Salesperson Problem (TSP) solver using a branch and bound algorithm.

This algorithm is based on the concepts presented in Chapter 8 of [1], specifically Section 8.3, which describes the Least Cost Branch and Bound using static state space tree for the Traveling Salesperson Problem. The provided web reference offers helpful examples to understand how this algorithm operates.

References

@luanleonardo luanleonardo changed the title Feat: Solves TSP using branch and bound method Feat: solves TSP using branch and bound method Jul 22, 2023
@luanleonardo luanleonardo changed the title Feat: solves TSP using branch and bound method Feat: solve TSP using branch and bound method Jul 22, 2023
@luanleonardo luanleonardo marked this pull request as ready for review July 24, 2023 23:01
Comment on lines +40 to +63
@pytest.mark.parametrize(
"distance_matrix, expected_distance",
[
(distance_matrix1, optimal_distance1),
(distance_matrix2, optimal_distance2),
(distance_matrix3, optimal_distance3),
],
)
def test_solution_is_optimal(distance_matrix, expected_distance):
"""
Test the `solve_tsp_branch_and_bound` function for optimality.

Verifies if the exact method returns an optimal solution.

The function is tested with three different distance matrices and their
corresponding optimal distances.

Verifications:
- The distance returned by the function should be equal to the
expected optimal distance.
"""
_, distance = solve_tsp_branch_and_bound(distance_matrix)

assert distance == expected_distance
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

author's note: For test case 2 the algorithm finds the permutation [0, 1, 4, 3, 2] also with an optimal cost of 21, but different from the expected permutation that it fixed at [0, 1, 2, 4, 3]. That's why I didn't make claims about equality of permutations. But test_solution_has_all_nodes guarantees that solutions found for these same entries are valid permutations!

Copy link
Owner

@fillipe-gsm fillipe-gsm left a comment

Choose a reason for hiding this comment

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

Very good work here!

Apart from the relatively minor comments, I have a question: did you benchmark the other exact solvers with the branch and bound? It is mentioned that it is more scalable than the existing ones, and I would love to know by how much.

Maybe a since benchmark like this could suffice:

import numpy as np

from python_tsp.exact import solve_tsp_brute_force
from python_tsp.exact import solve_tsp_dynamic_programming
from python_tsp.exact import solve_tsp_branch_and_bound

solvers = [
    solve_tsp_brute_force, 
    solve_tsp_dynamic_programming,
    solve_tsp_branch_and_bound,
]

problem_sizes = [3, 4, 5, 6, 7, 8, 9, ..., 15]  # maybe up to 20 could be more than enough

for solver in solvers:
    for n in problem_sizes:
        distance_matrix = #  build a random distance matrix with np.random and setting main diagonal to zero
        # Measure time and solution quality and add it to a table
        _, fopt = solver(distance_matrix)

This is not required for the approval of this PR, we can think about this later in a separate issue.

The only current blocking comment is the possibility to handle distance matrices with float numbers.

python_tsp/exact/__init__.py Show resolved Hide resolved
live_node = Node.from_parent(parent=min_node, index=index)
pq.push(live_node)

return [], 0
Copy link
Owner

Choose a reason for hiding this comment

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

This seems to be a behavior unique to this solver. All of the other solvers will return a valid permutation anyway, even it its cost is extremely large.

I think it's okay in this case, but instead of returning 0 as objective value I think one of the two would be better:

  • Return inf, in this case, from the math library, since we expect this variable to be a float;
  • Or raise an exception. If the solver only fails when the distance matrix is invalid then this may be better as it indicates an error by the user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fillipe-gsm I prefer to return inf, the reason was added as a note to the solver:

Notes
-----
The `distance_matrix` should be a square matrix with non-negative
values. The element `distance_matrix[i][j]` represents the distance from
city `i` to city `j`. If two cities are not directly connected, the
distance should be set to a float value of positive infinity
(float('inf')).
The path is represented as a list of city indices, and the total cost is a
float value indicating the sum of distances in the optimal path.
If the TSP cannot be solved (e.g., due to disconnected cities), the
function will return an empty path ([]) and a cost of positive infinity
(float('inf')).

"""
num_cities = len(distance_matrix)
cost_matrix = np.copy(distance_matrix)
inf = np.iinfo(cost_matrix.dtype).max
Copy link
Owner

Choose a reason for hiding this comment

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

From what I can get from the documentation, this method only works for integers.

It is mentioned in the README that the solvers in this library can handle matrices with floats, but I think with this line the function would fail if the distance matrix is not of integer type.

Can your code work floating point numbers as well? If so, maybe replacing this inf with the regular inf from the math module would be enough.

If not, tell me and we can figure something out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Totally right @fillipe-gsm, it only worked for integers, I've fixed this behavior and now it works for distance matrix with float numbers too.

python_tsp/exact/branch_and_bound/node.py Outdated Show resolved Hide resolved
@luanleonardo
Copy link
Collaborator Author

Very good work here!

Apart from the relatively minor comments, I have a question: did you benchmark the other exact solvers with the branch and bound? It is mentioned that it is more scalable than the existing ones, and I would love to know by how much.

Maybe a since benchmark like this could suffice:

import numpy as np

from python_tsp.exact import solve_tsp_brute_force
from python_tsp.exact import solve_tsp_dynamic_programming
from python_tsp.exact import solve_tsp_branch_and_bound

solvers = [
    solve_tsp_brute_force, 
    solve_tsp_dynamic_programming,
    solve_tsp_branch_and_bound,
]

problem_sizes = [3, 4, 5, 6, 7, 8, 9, ..., 15]  # maybe up to 20 could be more than enough

for solver in solvers:
    for n in problem_sizes:
        distance_matrix = #  build a random distance matrix with np.random and setting main diagonal to zero
        # Measure time and solution quality and add it to a table
        _, fopt = solver(distance_matrix)

This is not required for the approval of this PR, we can think about this later in a separate issue.

The only current blocking comment is the possibility to handle distance matrices with float numbers.

@fillipe-gsm Loved the idea, did the following benchmark from the attached screenshot, saved here. You can see that as n gets bigger, the BB solver is faster!

image

@luanleonardo luanleonardo requested a review from fillipe-gsm July 29, 2023 16:05
Copy link
Owner

@fillipe-gsm fillipe-gsm left a comment

Choose a reason for hiding this comment

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

Very good job, and thanks for the contribution!

I am merging a PR updating dependencies and after that I'll upload version 0.4.0 with your algorithm 💟

@fillipe-gsm fillipe-gsm merged commit bc4c941 into fillipe-gsm:master Aug 2, 2023
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.

2 participants