-
Notifications
You must be signed in to change notification settings - Fork 14
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
Make nlp
a concretely typed field in MadNLPSolver
#220
Conversation
Before this PR, all accesses to fields on `ips.nlp` were type unstable and caused allocations.
Codecov Report
@@ Coverage Diff @@
## master #220 +/- ##
=======================================
Coverage 74.33% 74.33%
=======================================
Files 37 37
Lines 3674 3674
=======================================
Hits 2731 2731
Misses 943 943
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
This overall looks good to me. It was something on our todo list, but haven't found the time to fix this. The question is about whether we should allow MadNLP to support very corner cases, as changing entirely the model between two consecutive solves. I guess the answer is not.
Medium-term, it would be nice to update the structure to be immutable
as well.
src/IPM/IPM.jl
Outdated
@@ -5,8 +5,8 @@ abstract type AbstractMadNLPSolver{T} end | |||
|
|||
include("restoration.jl") | |||
|
|||
mutable struct MadNLPSolver{T,KKTSystem <: AbstractKKTSystem{T}} <: AbstractMadNLPSolver{T} | |||
nlp::AbstractNLPModel | |||
mutable struct MadNLPSolver{T,KKTSystem <: AbstractKKTSystem{T}, N <: AbstractNLPModel} <: AbstractMadNLPSolver{T} |
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 would suggest renaming the type N
as Model
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.
This has been done in
bb93607
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.
Looks good to me! Thanks for the PR @baggepinnen
Before this PR, all accesses to fields on
ips.nlp
were type unstable and caused allocations.