-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add MOI methods: delete and modify #584
Conversation
Codecov Report
@@ Coverage Diff @@
## master #584 +/- ##
==========================================
- Coverage 86.82% 86.65% -0.18%
==========================================
Files 47 47
Lines 4752 4796 +44
==========================================
+ Hits 4126 4156 +30
- Misses 626 640 +14
Continue to review full report at Codecov.
|
push!(varids, varid) | ||
end | ||
for varid in varids | ||
coefmatrix[constrid, varid] = 0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if you do :
for (varid, _) in @view coefmatrix[constrid, :]
coefmatrix[constrid, varid] = 0.0
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it enters in an undefined behavior because the size of the array that stores the coefficient matrix may change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, I got an error for accessing an invalid index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to create methods to delete variable and constraint in MathProg.
So we would have :
delete!(formulation, var)
delete!(formulation, varid)
delete!(formulation, constr)
delete!(formulation, constrid)
|
||
function Base.delete!(form::Formulation, constrid::ConstrId) | ||
delete!(form.buffer.constr_buffer.added, constrid) | ||
delete!(form.manager.constrs, constrid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete!(form.manager.constrs, constrid) | |
delete!(form.manager.constrs, constrid) | |
return |
for (varid, _) in @view coefmatrix[constrid, :] | ||
push!(varids, varid) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should change the coefficients in MathProg.delete!
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah indeed
I need these changes to fix #583 so I merge into master. |
* add delete and modify methods for constraints * add delete and modify methods for variables * fix merge commit * add delete! and use proper get and set methods * apply suggestions from PR #584 Co-authored-by: Guillaume Marques <[email protected]>
No description provided.