Revisit fixed variables treatment #360
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Change the behavior of MakeParameter, depending on the callback:
- `SparseCallback`: remove the fixed variables and reduce problem's dimension in MadNLP
- `DenseCallback`: keep the fixed variables frozen (as done previously)
* Add a new `fixed_handler` if the problem has no fixed variable: `NoFixedVariable`.
* Ensure we always interact to the nonlinear program `nlp` through the
callback wrapper `cb` to avoid side effect.
* Remove the function `get_index_constraint` and stores indexes directly
in the callback wrapper `cb`.
* Fix the computation of the bound's multipliers for fixed variables.
|
Results on CUTEst are out (comparing this PR with MadNLP 0.8.4). It looks like we just improve very slightly the total number of iterations. In terms of computation time, we are slower. Also we are less robust now than before. Currently investigating what's going on. |
|
Benchmark is improved if we use the fix in #361 in this PR: |
sshin23
approved these changes
Nov 13, 2024
|
|
||
| nnzh = length(ind_hess_free) | ||
| Hi, Hj = similar(hess_I, nnzh), similar(hess_J, nnzh) | ||
| Hi .= map_full_to_free[hess_I[ind_hess_free]] |
|
|
||
| nnzj = length(ind_jac_free) | ||
| Ji, Jj = similar(jac_I, nnzj), similar(jac_J, nnzj) | ||
| Ji .= jac_I[ind_jac_free] |
|
|
||
| if m > 0 | ||
| if equality_treatment == EnforceEquality | ||
| ind_eq = findall(lcon .== ucon) |
There was a problem hiding this comment.
is_equality = (lcon .== ucon)
ind_eq = findall(is_equality)
ind_ineq = findall(~, is_equality)
| end | ||
|
|
||
| ind_llb = findall((lvar .!= -Inf) .* (uvar .== Inf)) | ||
| ind_uub = findall((lvar .== -Inf) .* (uvar .!= Inf)) |
There was a problem hiding this comment.
is_upper_unbounded = uvar .== Inf
is_lower_unbounded = lvar .==-Inf
ind_llb = .~ (is_lower_unbounded) .* is_upper_unbounded
ind_uub = (is_lower_unbounded) .* .~(is_upper_unbounded)| equality_handler = equality_treatment() | ||
|
|
||
| # Get indexing |
There was a problem hiding this comment.
if this a repeatition from SparseCallback, I suggest writing this as a function
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SparseCallback: remove the fixed variables and reduce problem's dimension in MadNLPDenseCallback: keep the fixed variables frozen (as done previously). On the contrary toSparseCallback, we don't remove the fixed variables as this would require duplicating the Hessian and the Jacobian matrices to evaluate the model, which could be expensive if we are memory bounded.fixed_handlerif the problem has no fixed variable:NoFixedVariable.nlpthrough the callback wrappercbto avoid side effect.get_index_constraintand stores indexes directly in the callback wrappercb.Solve #70, #229
Warning: this PR adds a small breaking change in
create_kkt_system(we removeind_consfrom the arguments) but I think it's worth the change, as the previous situation was error prone. Now, we make sure we store all the problem's information inside the callback wrappercb.Preliminary results on
space_shuttle_reentryshows that the current PR improves slightly the convergence (less iterations, less feasibility restoration), see the logs in attachment:Benchmark on CUTEst is under way.