Skip to content

Integrate GLKH-Solver - #56

Open
kledom wants to merge 3 commits into
ethz-asl:masterfrom
kledom:helsgaun
Open

Integrate GLKH-Solver#56
kledom wants to merge 3 commits into
ethz-asl:masterfrom
kledom:helsgaun

Conversation

@kledom

@kledom kledom commented Aug 10, 2021

Copy link
Copy Markdown
Contributor

Removes mono dependency by integrating Helsgaun's GLKH-Solver (http://webhotel4.ruc.dk/~keld/research/GLKH/).

Helsgaun, K.: Solving the equality generalized traveling salesman problem using the Lin–Kernighan–Helsgaun algorithm. Mathematical Programming Computation7(3),269–287 (2015)

Solves #24.

@rikba

rikba commented Aug 12, 2021

Copy link
Copy Markdown
Collaborator

Nice @kledom ! What's the status on this? Should I test this?

@kledom

kledom commented Aug 30, 2021

Copy link
Copy Markdown
Contributor Author

Hi @rikba,

sorry for the late response. I was on vacation for the last 2 weeks.

In general, the implementation works but I need to do some more testing.
Also, I didn't implement the method setSolver(const std::string& file, bool binary) yet, which is probably the reason for the failing tests.

I'll let you know as soon as it's ready.

@kledom

kledom commented Sep 9, 2021

Copy link
Copy Markdown
Contributor Author

@rikba Ready for review!

@kledom
kledom marked this pull request as ready for review September 9, 2021 14:54
m[i][j] = doubleToMilliInt(cost);
} else {
m[i][j] = std::numeric_limits<int>::max();
m[i][j] = std::numeric_limits<int>::max(); // TODO: overflow!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rikba You might want to have a look at this line. Setting the weights to std::numeric_limits<int>::max() causes an integer overflow within GLKH. What value should we use instead?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If I remember correctly std::numeric_limits<int>::max()was used in gkma to note edges in the adjacency matrix that are not traversable. I had a look into the GTSPLIB instances in GLKH-1.0/GTSPLIB and they have different large values there, e.g., 10ftv47.gtsp has 100000000, 4br17.gtsp has 9999, 11ft53.gtsp has 9999999. So maybe it's ok to allocate 9999999 like you do below but add a TODO that we want to setup the problem in EDGE_DATA_FORMAT and let GKLH handle the disconnected edges.

Comment on lines +89 to +95
for (auto w_ij : task.m[i])
{
if (w_ij == 2147483647)
{
// TODO: FIX Overflow
w_ij = 9999999;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is my current hotfix (should be removed before merging).

@rikba rikba left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @kledom. This looks a lot cleaner than the GKMA approach with mono. I'll test this probably beginning of October and may add some benchmarking to compare it to GKMA.

m[i][j] = doubleToMilliInt(cost);
} else {
m[i][j] = std::numeric_limits<int>::max();
m[i][j] = std::numeric_limits<int>::max(); // TODO: overflow!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If I remember correctly std::numeric_limits<int>::max()was used in gkma to note edges in the adjacency matrix that are not traversable. I had a look into the GTSPLIB instances in GLKH-1.0/GTSPLIB and they have different large values there, e.g., 10ftv47.gtsp has 100000000, 4br17.gtsp has 9999, 11ft53.gtsp has 9999999. So maybe it's ok to allocate 9999999 like you do below but add a TODO that we want to setup the problem in EDGE_DATA_FORMAT and let GKLH handle the disconnected edges.

@kledom

kledom commented Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

@rikba I just rebased the commits to the current master. Just in case you are still considering merging this.

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