-
Notifications
You must be signed in to change notification settings - Fork 20
Overhaul solver member access via functions #497
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #497 +/- ##
==========================================
- Coverage 84.63% 84.56% -0.07%
==========================================
Files 49 51 +2
Lines 4400 4413 +13
==========================================
+ Hits 3724 3732 +8
- Misses 676 681 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6278351 to
e63295e
Compare
|
Just a note: I cannot reproduce the |
|
@aspozharski could be due to the new release of julia v1.12 |
frapac
left a comment
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 like the direction where this is going. Having getters would also help with the C interface.
I would suggest generating all the getters automatically using metaprogramming. E.g. using the following code:
for (k, attribute) in enumerate(fieldnames(MadNLPSolver))
fname = "get_$(attribute)"
@eval begin
function $(Symbol(fname))(solver::MadNLPSolver)
return getfield(solver, $k)
end
end
end
|
While I agree that the metaprogramming is nice from a maintenance perspective I always have found it to be a pain for future developers because things are not explicitly e.g. greppable. |
Yep indeed there were some issues from the v1.12 release. #498 should fix these. |
4eb5f84 to
71cc795
Compare
71cc795 to
bd99177
Compare
54b40ea to
1a4d659
Compare
The goal of this PR is to attempt to minimize the number of structure dependent
solver.*calls in core MadNLP.The idea here is it easier in principle to create alternate subclasses of
AbstractMadNLPSolverthat only need to specialize some or all of the API found inIPM/api.jland in doing so can make maximum use of the existing code in MadNLP with minimal required duplication.I suspect there is more to be done in this direction, e.g. wrapping more "calculated" quantities over the current solver state, into functions
_quantity(solver::AbstractMadNLPSolver) = <expression>, but I believe this is a good first pass.(Note, that I am not married to the naming scheme etc., of this PR, if you would prefer
set_<field>(solver::AbstractMadNLPSolver)andget_<field>(solver::AbstractMadNLPSolver), I would be happy with that as well, however due to shadowing pain I think just<field>(solver::AbstractMadNLPSolver)is not the best idea.)