Conversation
| for i in range(nz): | ||
| v_idx = expr.vars[i].index | ||
| v_coeff = expr.coefficients[i] | ||
| self.vindex_coeff_dict[v_idx] = self.vindex_coeff_dict[v_idx] + v_coeff if v_idx in self.vindex_coeff_dict else v_coeff |
There was a problem hiding this comment.
My guess is that the speed of this loop can be improved.
The hash lookup is likely costly. Suppose you have an array of size n (where n is the number of variables) that is all zero. You could store that array in the problem.
workspace = problem.workspace # workspace is all zero and of length variables
# scatter the nonzero coefficients into workspace.
# Any coefficients with the same variable index will get added
self.indices = []
for i in range (nz):
j = expr.vars[i].index
x = expr.coefficients[i]
if workspace[j] == 0.0:
self.indices.append(j)
workspace[j] += x
# gather the nonzero coefficients into self.coefficients
final_nz = len(self.indices)
self.coefficients = []
for k in range(final_nz):
self.coefficients.append(workspace[self.indicies[k]])
# clear workspace
for k in range(final_nz):
workspace[self.indices[k]] = 0.0
This should be fast in C/C++. But might be slow in Python.
There was a problem hiding this comment.
We can have this as an evaluation and improvement in the next cycle. It looks promising but not a significant improvement. Needs further perf results.
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Awesome work @chris-maes and @Iroy30, I have initial few comments.
Also lets add pytests to cover all the APIs and also scenarios where problem gets modified in a succession so the addition and deletion can be tested to make sure data is getting updated.
@rgsl888prabhu deletion is not supported with this API. You can only add new variables or add new constraints. @Iroy30 for addition of variables and constraints we might want to reconsider the design a bit. Adding a new variable to a problem should probably invalidate the |
|
/ok to test a27fb80 |
|
/ok to test 7562bbe |
|
/ok to test d71b77e |
| # Set the incumbent callback | ||
| incumbent_callback = IncumbentCallback() | ||
| settings.set_mip_callback(incumbent_callback) | ||
| settings.set_parameter("time_limit", 30) # Allow enough time to find multiple incumbents |
There was a problem hiding this comment.
Let's not use a bare string here. Let's use CUOPT_TIME_LIMIT instead.
There was a problem hiding this comment.
And also other places where we use time_limit? And may be also update test case to do same thing.
There was a problem hiding this comment.
that looks to be the only place in docs
|
/ok to test 8ae3746 |
|
/ok to test e6ade53 |
chris-maes
left a comment
chris-maes
left a comment
There was a problem hiding this comment.
LGTM. I think we should merge as is. We can follow up with some doc improvements before the release.
|
/ok to test 2c0a1ab |
|
/ok to test f5c127d |
|
/ok to test a8a6515 |
|
/ok to test 1967a3d |
|
/merge |
## Issue Authors: - Ishika Roy (https://github.com/Iroy30) - Trevor McKay (https://github.com/tmckayus) - Ramakrishnap (https://github.com/rgsl888prabhu) Approvers: - Trevor McKay (https://github.com/tmckayus) - Chris Maes (https://github.com/chris-maes) URL: NVIDIA#223
4 participants
Description
Issue
Checklist
Working on Adding tests and solution extraction