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

Speed up computation of reduced costs #309

Merged
merged 3 commits into from
Apr 15, 2020
Merged

Conversation

guimarqu
Copy link
Contributor

No description provided.

Copy link
Collaborator

@rrsadykov rrsadykov left a comment

Choose a reason for hiding this comment

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

Ok, it is a little bit better, but still it is slow.
On the same instance gapC-10-100.txt calculation of reduced costs takes 16 seconds and allocates 5 Gb of memory. I do not understand why so much memory is allocated.

For comparison, in BapCod, calculation of reduced costs (normalized to the same number of iterations) takes 115 milliseconds. Part of the reason is that in BaPCod, we compare the length of the dual solution and the number of non-zeros of subproblem variable in the matrix. If the latter is smaller, we make a loop over these non-zeros, and not over the dual solution.
But, anyway, even if I force in BapCod to loop over the dual solution, the time increases to 760 milliseconds, which is 20 times(!) faster than Coluna.

We need to do something, this is critical for me.

@guimarqu guimarqu requested a review from rrsadykov April 15, 2020 12:40
@guimarqu
Copy link
Contributor Author

Do you compute c - yA ? If yes, you need to loop over the matrix non-zeros & the dual solution.

@rrsadykov
Copy link
Collaborator

Yes, the first variant is to loop over the dual solution (over constraints in the dual solution) and then on every iteration to look for the coefficient of the variable in the constraint (you search in the map for this).
The second variant is to loop over the membership of the variable in the master (over constraints which involve the variable) and then on every iteration to look for the value of this constraint in the dual solution (you search the dual solution for this)

@rrsadykov
Copy link
Collaborator

rrsadykov commented Apr 15, 2020

I have just looked, 95% of time and almost all memory is taken in this line:

for (constrid, val) in dualsol

@guimarqu
Copy link
Contributor Author

I have just looked, 95% of time and almost all memory is taken in this line:

for (constrid, val) in dualsol

I just changed the code because it was an useless nested loop.

@rrsadykov
Copy link
Collaborator

Ok, good! Now it takes 58 milliseconds.
Do you think it will take much time to clean the code? It would be nice also to separate the reduced cost calculation per subproblem. In the future, we may want to solve only a subset of subproblems on a col.gen. iteration, if the number of subproblems is large.

@rrsadykov
Copy link
Collaborator

Another performance issue is with function setprimalsol! in src/MathProg/formulation.jl
It takes more than 1 second for less than 2000 calls. This is less critical but it should later be addressed. In BaPCod the verification if a column already exists is done using the lexicographic order of colunms.

@guimarqu
Copy link
Contributor Author

Code is clean.

About the computations of reduced costs for a given subproblem : I guess it's the 2nd variant, we'll do it when needed.

About the setprimalsol! : I opened a new issue #310

@rrsadykov
Copy link
Collaborator

The code in updatereducedcosts!() is too low-level for me. I think we need some interface here.
Is it easy to do or we should leave it for later?

@guimarqu
Copy link
Contributor Author

The code in updatereducedcosts!() is too low-level for me. I think we need some interface here.
Is it easy to do or we should leave it for later?

Yes, it should be in DynamicSparseArrays. We leave it for later.

@rrsadykov
Copy link
Collaborator

Guillaume, can you merge this first, and then my pull request?

@guimarqu guimarqu merged commit 26ca2a2 into master Apr 15, 2020
@guimarqu guimarqu deleted the computeredcostcolgen branch April 15, 2020 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants