Remove StrideOneVector alias #174
Conversation
- StrideOneVector lead to type inference issues
lib/MadNLPHSL/src/ma27.jl
Outdated
| @@ -48,7 +48,7 @@ mutable struct Solver <: AbstractLinearSolver | |||
| logger::Logger | |||
| end | |||
|
|
|||
| ma27ad!(n::Cint,nz::Cint,I::StrideOneVector{Cint},J::StrideOneVector{Cint}, | |||
| ma27ad!(n::Cint,nz::Cint,I::AbstractVector{Cint},J::AbstractVector{Cint}, | |||
There was a problem hiding this comment.
I agree that StrideOneVector is not ideal, but the motivation for using it was to make sure that when calling linear solvers, we don't accidentally pass AbstractVectors with non-uniform memory stride (in particular, we want it to be always 1 byte). My concern with this change is that we don't have anything that prevents ma27ad! to accept a pointer to a non-stride-one-array. Of course, we're using these only internally within MadNLP, so shouldn't be a problem if we write the code carefully in the future, but it raises a bit of concern to me. Any thoughts on this @frapac?
There was a problem hiding this comment.
To be honest, I have exactly the same concern as you. I thought about this the last few days, and I may have a potential workaround. How about adding a safety check at the beginning of the function:
@assert strides(I) == (1, )
|
@frapac can you take a look? |
src/IPM/callbacks.jl
Outdated
| function eval_f_wrapper(ips::InteriorPointSolver, x::Vector{Float64}) | ||
| nlp = ips.nlp | ||
| cnt = ips.cnt | ||
| @trace(ips.logger,"Evaluating objective.") | ||
| cnt.eval_function_time += @elapsed obj_val = (get_minimize(nlp) ? 1. : -1.) * obj(nlp,view(x,1:get_nvar(nlp))) | ||
| cnt.eval_function_time += @elapsed obj_val = (get_minimize(nlp) ? 1. : -1.) * obj(nlp,unsafe_wrap(Vector{Float64},pointer(x),get_nvar(nlp))) |
There was a problem hiding this comment.
To improve readability, I would move the unsafe_wrap in a line before the function call. Something like
x_primal = unsafe_wrap(Vector{Float64},pointer(x),get_nvar(nlp))
src/KKT/sparse.jl
Outdated
| jac::StrideOneVector{T} | ||
| pr_diag::StrideOneVector{T} | ||
| du_diag::StrideOneVector{T} | ||
| struct SparseKKTSystem{T, MT, VT} <: AbstractReducedKKTSystem{T, MT} |
src/KKT/sparse.jl
Outdated
| # Augmented system | ||
| aug_raw::SparseMatrixCOO{T,Int32} | ||
| aug_raw::SparseMatrixCOO{T,Int32,Vector{T}} |
Codecov Report
@@ Coverage Diff @@
## master #174 +/- ##
==========================================
+ Coverage 72.73% 72.78% +0.04%
==========================================
Files 38 38
Lines 3418 3424 +6
==========================================
+ Hits 2486 2492 +6
Misses 932 932
Continue to review full report at Codecov.
|
StrideOneVector lead to type inference issues in MadNLP. This PR replaces
StrideOneVectorby using explicit typing in the impacted structure (SparseKKTSystemandSparseUnreducedKKTSystem). This PR might improve the time to first solve in MadNLP.