Skip to content

Conversation

@apozharski
Copy link
Contributor

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 AbstractMadNLPSolver that only need to specialize some or all of the API found in IPM/api.jl and 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) and get_<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.)

@frapac frapac self-requested a review October 8, 2025 14:36
@codecov
Copy link

codecov bot commented Oct 8, 2025

Codecov Report

❌ Patch coverage is 90.08000% with 62 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.56%. Comparing base (8a2fee6) to head (287dd4d).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/IPM/solver.jl 86.03% 37 Missing ⚠️
src/IPM/line_search.jl 78.20% 17 Missing ⚠️
src/IPM/types.jl 68.75% 5 Missing ⚠️
src/IPM/api.jl 62.50% 3 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@apozharski
Copy link
Contributor Author

Just a note: I cannot reproduce the test-madnlp failure locally on my machine, which is odd.

@sshin23
Copy link
Member

sshin23 commented Oct 9, 2025

@aspozharski could be due to the new release of julia v1.12

Copy link
Member

@frapac frapac left a 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

@apozharski
Copy link
Contributor Author

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.

@apozharski
Copy link
Contributor Author

@aspozharski could be due to the new release of julia v1.12

Yep indeed there were some issues from the v1.12 release. #498 should fix these.

@apozharski apozharski force-pushed the ap/solver-api branch 3 times, most recently from 4eb5f84 to 71cc795 Compare October 13, 2025 06:13
@apozharski apozharski requested a review from frapac November 6, 2025 09:40
@frapac frapac mentioned this pull request Jan 7, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants