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

package name confict #3

Closed
dribnet opened this issue May 24, 2017 · 5 comments · Fixed by #6
Closed

package name confict #3

dribnet opened this issue May 24, 2017 · 5 comments · Fixed by #6

Comments

@dribnet
Copy link
Contributor

dribnet commented May 24, 2017

Would you consider changing the name of this package? It has a naming conflict with this lapjv which is pip installable. I have a proof of concept here where I have renamed it lapjv1, and it works fine. Happy to contribute that or rename it to something else.

Separately but relatedly, would you consider changing the API to return arguments to this:

rowsol_c, colsol_c, cost = lapjv(cost_matrix)

This would match the API of the other lapjv implementation so that they could be swapped out programatically.

@gatagat
Copy link
Owner

gatagat commented May 24, 2017

Yes, to both if you are able to create a PR.

Out of curiosity, what is the reason not to use this package exclusively (a missing pypi entry)?

BTW, the other lapjv has a shady license as it uses the original MagicLogic code which is under a restrictive license.

@gatagat
Copy link
Owner

gatagat commented May 24, 2017

The rename would actually make sense also because now there is not only the original lapjv algorithm for dense matrices but also the lapmod for sparse matrices. Let's rename it to "lap".

dribnet added a commit to dribnet/python-lap that referenced this issue Aug 15, 2017
fixes gatagat#3 - conflict with existing lapjv in pypi
@dribnet
Copy link
Contributor Author

dribnet commented Aug 15, 2017

Hi again. I've finally renamed the library again from lapjv to lap in #6 - please let me know what you think. I've tested this with a downstream dependency and it works, but I didn't run any of the unit tests so it's not fully tested.

To answer your other question: the main reason to not use this package exclusively is that for me it seems to hang on larger problems. I can run a cost matrix with lapjv to 500x500 or so in about 8 seconds (and get great results), but if I try 700x700 it doesn't return for at least a few minutes. I can try to debug further if you are curious.

gatagat pushed a commit that referenced this issue Aug 27, 2017
fixes #3 - conflict with existing lapjv in pypi
@gatagat
Copy link
Owner

gatagat commented Aug 27, 2017

Thanks for the PR, it really helped, I just needed to make some small changes such as removing the cython output cpp.

Concerning the runtime. I am definitely interested in the matrices that trigger the long runtimes. I use the library routinely with >1000x1000 matrices (dense) and >15000x15000 (sparse), usually it runs in a fraction of a second. If it runs significantly more than a second, I'd consider it a bug. I've made some changes now that might improve the situation. Could you give it try and let me know?

@gatagat
Copy link
Owner

gatagat commented Aug 27, 2017

If you wanted to debug yourself, I would start with looking at the augmenting row reduction. it is one preprocessing step that can get very long to finish and although it usually helps, it is not absolutely necessary.

vlad-nn added a commit to DeGirum/lap that referenced this issue Apr 12, 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 a pull request may close this issue.

2 participants