Add farthest-insertion algorithm for the TSP #1105

alex-cornejo · 2021-09-06T20:22:13Z

Issue: #1104.
Add an implementation of the farthest-insertion heuristic for the Traveling Salesman Problem (TSP).

For the purpose of coding this algorithm with good quality, this implementation takes some performance ideas from other algorithm implementations such as NearhestInsertionTSP, NearhestNeighborTSP, and FloydWharsall.

This implementation has a similar structure to other algorithms implementations for the TSP already implemented in jGrapht.

According to some references and experimentation I performed, in several cases, this algorithm outperforms the others regarding running time and quality of found solutions.

Please see the linked issue #1104 to check additional details and a small comparison I performed.

Some references:

D. Rosenkrantz, R. Stearns, and P. II, "An analysis of several heuristics for the traveling salesman problem," SIAM J. Comput., vol. 6, pp. 563{581, 09 1977.
Skiena, S.-S. (2020). The Algorithm Design Manual (3rd ed.). Springer International Publishing.

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

alex-cornejo · 2021-10-04T20:58:11Z

Any interest in this?

micycle1 · 2023-11-10T03:03:23Z

It's rather good. Images below compare farthest-insertion vs nearest-neighbour. It's ~3x faster with a better result too!

alex-cornejo · 2024-01-07T10:51:57Z

@jkinable any interest on this?

syoon2 · 2024-03-17T03:26:44Z

Just FYI - the tests would not "run" after merging because JGraphT now uses JUnit 5 and your tests are written in JUnit 4.
~~By looking at your tests it seems that it would be as simple as replacing the JUnit 4 imports with the JUnit 5 equivalents though.~~

Edit: After taking a closer look, some tests require more than just editing the imports; specifically, the expected attribute is no longer supported in JUnit 5's @Test annotation.

alex-cornejo · 2024-03-17T04:44:32Z

It's rather good. Images below compare farthest-insertion vs nearest-neighbour. It's ~3x faster with a better result too!

Did you use a software to plot these tours?

alex-cornejo · 2024-03-17T04:51:09Z

Just FYI - the tests would not "run" after merging because JGraphT now uses JUnit 5 and your tests are written in JUnit 4. By looking at your tests it seems that it would be as simple as replacing the JUnit 4 imports with the JUnit 5 equivalents though.

Thanks so much for the observation. I can update the PR for JUnit 5; nevertheless, before any effort, it would be great to know if the JGrapht team is interested in integrating this PR since it has not been merged for a long time, maybe because it is not of interest.
@jkinable

UPDATE
I updated the unit tests to jUnit5.

syoon2 · 2024-03-17T05:39:01Z

@jsichi would you mind taking a look?

Previous implementation was based on the jgrapht implementation of Nearest Neighbor. Now it was changed and now is based on Nearest Insertion.

micycle1 · 2024-03-18T23:22:41Z

It's rather good. Images below compare farthest-insertion vs nearest-neighbour. It's ~3x faster with a better result too!

Did you use a software to plot these tours?

My own software that uses Processing for graphics.

jsichi

Thank you for your incredible patience! I will merge this after the swap methods are made private.

jsichi · 2024-03-21T05:11:27Z

jgrapht-core/src/main/java/org/jgrapht/alg/tour/FarthestInsertionHeuristicTSP.java

+     * @param i the index of the first element
+     * @param j the index of the second element
+     */
+    public static void swap(double[] arr, int i, int j)


Make this private

What do you think about moving the swap methods to the org.jgrapht.util.ArrayUtil class?

Yes that's better!

Thanks so much for your feedback. Done.

jsichi · 2024-03-21T05:11:36Z

jgrapht-core/src/main/java/org/jgrapht/alg/tour/FarthestInsertionHeuristicTSP.java

+     * @param i the index of the first element
+     * @param j the index of the second element
+     */
+    public static void swap(int[] arr, int i, int j)


This one too.

syoon2

Just few documentation stuff that came up to me while reading the changes:

I think that @since tags should be added to Javadoc for the new classes / methods (see suggestions)
I don't think the license headers for the two new files are accurate? It isn't 2021 now for sure...

Disclaimer: I am not associated with the maintainer(s) of the JGraphT repository, nor do I claim to be.

syoon2 · 2024-03-22T00:01:32Z

jgrapht-core/src/main/java/org/jgrapht/alg/tour/FarthestInsertionHeuristicTSP.java

+ * @param <V> the graph vertex type
+ * @param <E> the graph edge type
+ * @author José Alejandro Cornejo Acosta
+ */


Suggested change

* @param <V> the graph vertex type

* @param <E> the graph edge type

* @author José Alejandro Cornejo Acosta

*/

* @param <V> the graph vertex type

* @param <E> the graph edge type

* @author José Alejandro Cornejo Acosta

*

* @since 1.5.3

*/

syoon2 · 2024-03-22T00:02:55Z

jgrapht-core/src/test/java/org/jgrapht/alg/tour/FarthestInsertionHeuristicTSPTest.java

+/**
+ * Unit tests for the {@link FarthestInsertionHeuristicTSP}
+ *
+ * @author Jose Alejandro Cornejo Acosta
+ */


Suggested change

/**

* Unit tests for the {@link FarthestInsertionHeuristicTSP}

*

* @author Jose Alejandro Cornejo Acosta

*/

/**

* Unit tests for the {@link FarthestInsertionHeuristicTSP}

*

* @author Jose Alejandro Cornejo Acosta

*

* @since 1.5.3

*/

jgrapht-core/src/main/java/org/jgrapht/util/ArrayUtil.java

alex-cornejo · 2024-03-22T01:05:41Z

Just few documentation stuff that came up to me while reading the changes:
* I think that `@since` tags should be added to Javadoc for the new classes / methods (see suggestions)

* I don't think the license headers for the two new files are accurate? It isn't 2021 now for sure...
Disclaimer: I am not associated with the maintainer(s) of the JGraphT repository, nor do I claim to be.

Interesting. I agree with you about the dates. Let's

Just few documentation stuff that came up to me while reading the changes:
* I think that `@since` tags should be added to Javadoc for the new classes / methods (see suggestions)

* I don't think the license headers for the two new files are accurate? It isn't 2021 now for sure...
Disclaimer: I am not associated with the maintainer(s) of the JGraphT repository, nor do I claim to be.

Interesting. I agree with you about the dates, but the instructions say not to change the header. @jsichi What do you think about it?

jsichi · 2024-03-22T01:21:01Z

As it says in:

https://github.com/jgrapht/jgrapht/wiki/Dev-guide%3A-How-to-make-your-first-(code)-contribution

The only modifications you are allowed to make to the header are the year, and your name on the first line. The first line of the header should read:
* (C) Copyright <current year>-<current year>, by <your name> and Contributors.

Since these are new files, you should use your name (instead of copying the names from other files). For the year range, you can put 2021-2024 due to the long review period.

Thanks @syoon2 for noticing that.

alex-cornejo · 2024-03-22T02:41:51Z

As it says in:

https://github.com/jgrapht/jgrapht/wiki/Dev-guide%3A-How-to-make-your-first-(code)-contribution

The only modifications you are allowed to make to the header are the year, and your name on the first line. The first line of the header should read: * (C) Copyright <current year>-<current year>, by <your name> and Contributors.

Since these are new files, you should use your name (instead of copying the names from other files). For the year range, you can put 2021-2024 due to the long review period.

Thanks @syoon2 for noticing that.

I updated the headers. Thanks so much for the observation.

Co-authored-by: Sung Ho Yoon <55358516+syoon2@users.noreply.github.com>

jsichi · 2024-03-22T07:56:54Z

Thanks!

alex-cornejo added 5 commits March 18, 2024 12:54

first version

98a567a

details implementation changed to NI

f2cf2aa

Previous implementation was based on the jgrapht implementation of Nearest Neighbor. Now it was changed and now is based on Nearest Insertion.

unit tests added

59f8fed

coding conventions

8aef8b2

unit tests updated to jUnit5

5a72934

alex-cornejo force-pushed the farthest-insertion branch from 3ba835e to 5a72934 Compare March 18, 2024 23:04

jsichi reviewed Mar 21, 2024

View reviewed changes

swap functions moved to ArrayUtil

4ac2679

syoon2 reviewed Mar 22, 2024

View reviewed changes

headers updated

b136ecc

Apply suggestions from code review

b7f923f

Co-authored-by: Sung Ho Yoon <55358516+syoon2@users.noreply.github.com>

jsichi merged commit 5e72248 into jgrapht:master Mar 22, 2024
3 checks passed

alex-cornejo deleted the farthest-insertion branch March 24, 2024 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add farthest-insertion algorithm for the TSP #1105

Add farthest-insertion algorithm for the TSP #1105

alex-cornejo commented Sep 6, 2021 •

edited

alex-cornejo commented Oct 4, 2021

micycle1 commented Nov 10, 2023

alex-cornejo commented Jan 7, 2024 •

edited

syoon2 commented Mar 17, 2024 •

edited

alex-cornejo commented Mar 17, 2024

alex-cornejo commented Mar 17, 2024 •

edited

syoon2 commented Mar 17, 2024

micycle1 commented Mar 18, 2024

jsichi left a comment

jsichi Mar 21, 2024

syoon2 Mar 21, 2024

jsichi Mar 21, 2024

alex-cornejo Mar 21, 2024

jsichi Mar 21, 2024

syoon2 left a comment

syoon2 Mar 22, 2024

syoon2 Mar 22, 2024

alex-cornejo commented Mar 22, 2024 •

edited

jsichi commented Mar 22, 2024

alex-cornejo commented Mar 22, 2024

jsichi commented Mar 22, 2024

Add farthest-insertion algorithm for the TSP #1105

Add farthest-insertion algorithm for the TSP #1105

Conversation

alex-cornejo commented Sep 6, 2021 • edited

alex-cornejo commented Oct 4, 2021

micycle1 commented Nov 10, 2023

alex-cornejo commented Jan 7, 2024 • edited

syoon2 commented Mar 17, 2024 • edited

alex-cornejo commented Mar 17, 2024

alex-cornejo commented Mar 17, 2024 • edited

syoon2 commented Mar 17, 2024

micycle1 commented Mar 18, 2024

jsichi left a comment

Choose a reason for hiding this comment

jsichi Mar 21, 2024

Choose a reason for hiding this comment

syoon2 Mar 21, 2024

Choose a reason for hiding this comment

jsichi Mar 21, 2024

Choose a reason for hiding this comment

alex-cornejo Mar 21, 2024

Choose a reason for hiding this comment

jsichi Mar 21, 2024

Choose a reason for hiding this comment

syoon2 left a comment

Choose a reason for hiding this comment

syoon2 Mar 22, 2024

Choose a reason for hiding this comment

syoon2 Mar 22, 2024

Choose a reason for hiding this comment

alex-cornejo commented Mar 22, 2024 • edited

jsichi commented Mar 22, 2024

alex-cornejo commented Mar 22, 2024

jsichi commented Mar 22, 2024

alex-cornejo commented Sep 6, 2021 •

edited

alex-cornejo commented Jan 7, 2024 •

edited

syoon2 commented Mar 17, 2024 •

edited

alex-cornejo commented Mar 17, 2024 •

edited

alex-cornejo commented Mar 22, 2024 •

edited