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

Extension of non-square matrix with all negative values in lapjv #21

Open
jvlmdr opened this issue Feb 1, 2020 · 7 comments
Open

Extension of non-square matrix with all negative values in lapjv #21

jvlmdr opened this issue Feb 1, 2020 · 7 comments

Comments

@jvlmdr
Copy link

jvlmdr commented Feb 1, 2020

Hey, I think there is a bug in the code for extending a non-square matrix to a square matrix.

Consider the all-negative cost matrix

[[2, 4, 6, 8], [1, 2, 4, 8]] - 30

with values in the range [-30, -20].

The minimal cost should be 2 + 2 + -30 * 2 = -56 but lapjv returns 0. The indices of the solution are all -1.

@jvlmdr jvlmdr mentioned this issue Feb 1, 2020
@gatagat
Copy link
Owner

gatagat commented Feb 1, 2020

Hi, the cost matrix must be non-negative.

Do you see the error with cost + 30?

@jvlmdr
Copy link
Author

jvlmdr commented Feb 1, 2020

Oh ok, sorry, I didn't realise :)

@jvlmdr
Copy link
Author

jvlmdr commented Feb 1, 2020

And yep, it gives the correct solution for the positive cost matrix.

@gatagat
Copy link
Owner

gatagat commented Feb 1, 2020

This should definitely be mentioned in the docs. If you have some time, please, send a PR.

@jvlmdr
Copy link
Author

jvlmdr commented Feb 2, 2020

Maybe it would be better to also raise an exception if one of the costs is negative, or subtract the minimum? But are you sure it doesn't work with negative costs? It seems to give the correct result, although I haven't tested it extensively. (The modification that I proposed in #22 was only to fix an issue with the extension to a square cost matrix when all of the costs were negative.)

@gatagat
Copy link
Owner

gatagat commented Feb 13, 2020

@jvlmdr You are right, my bad, lack of sleep, lack of time... It is the dual where the reduced costs have to be non-negative, so no issue in the algorithm itself, the extension is wrong.

@gatagat
Copy link
Owner

gatagat commented Feb 13, 2020

And your patch #22 is sound. The only question that remains is what has a better runtime: extend the way you do, or shift the costs to zero first and extend as before?

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

No branches or pull requests

2 participants