Skip to content

Scipywrapper #927

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

Merged
merged 9 commits into from
Jun 20, 2025
Merged

Scipywrapper #927

merged 9 commits into from
Jun 20, 2025

Conversation

AdityaPandeyCN
Copy link
Contributor

@AdityaPandeyCN AdityaPandeyCN commented Jun 15, 2025

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

I have implemented OptimizationSciPy.jl wrapper addressing issue #917, providing access to scipy.optimize through Julia's Optimization.jl interface.
What I implemented:

  1. 25+ optimizer types covering local (BFGS, Nelder-Mead, trust-constr), global (differential_evolution, basinhopping, shgo, direct), root finding, and least squares methods
  2. I have tried to do full SciMLBase integration with proper traits, constraints, bounds, and callback support

I still think I can make it more better but reviews from the maintainers will take it in the correct direction.

Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
…nd bug fixes

Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
Comment on lines 154 to 172
struct ScipyLeastSquares
method::String
loss::String
function ScipyLeastSquares(; method::String="trf", loss::String="linear")
valid_methods = ["trf", "dogbox", "lm"]
valid_losses = ["linear", "soft_l1", "huber", "cauchy", "arctan"]
if !(method in valid_methods)
throw(ArgumentError("Invalid method: $method. Valid methods are: $(join(valid_methods, ", "))"))
end
if !(loss in valid_losses)
throw(ArgumentError("Invalid loss: $loss. Valid loss functions are: $(join(valid_losses, ", "))"))
end
new(method, loss)
end
end

ScipyLeastSquaresTRF() = ScipyLeastSquares(method="trf")
ScipyLeastSquaresDogbox() = ScipyLeastSquares(method="dogbox")
ScipyLeastSquaresLM() = ScipyLeastSquares(method="lm")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are for LeastSquaresProblem?

function SciMLBase.__init(prob::SciMLBase.OptimizationProblem, opt::Union{ScipyMinimize,
ScipyDifferentialEvolution, ScipyBasinhopping, ScipyDualAnnealing,
ScipyShgo, ScipyDirect, ScipyBrute, ScipyMinimizeScalar,
ScipyLeastSquares, ScipyRootScalar, ScipyRoot, ScipyLinprog, ScipyMilp};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just have a SciPyOptimizers abstract type on top to make this simpler.

end

function SciMLBase.__solve(cache::OptimizationCache{F,RC,LB,UB,LC,UC,S,O,D,P,C}) where
{F,RC,LB,UB,LC,UC,S,O<:ScipyRoot,D,P,C}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for NonlinearSolve.jl instead?

Copy link
Contributor Author

@AdityaPandeyCN AdityaPandeyCN Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want ScipyRoot to accept a NonlinearProblem directly (so users can call solve on it), or the wrappers should live inside NonlinearSolve.jl instead? I am confused in this

end
x0_ls = [1.0, 0.0]
optf = OptimizationFunction(residuals)
prob = OptimizationProblem(optf, x0_ls)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs a nonlinearleastsquaresproblem?

Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
…comments

Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
@AdityaPandeyCN
Copy link
Contributor Author

I have tried to address the suggestions @ChrisRackauckas

Comment on lines +205 to +209
@testset "ScipyRootScalar" begin
f_root(x, p) = x[1]^3 - 2*x[1] - 5
x0_root = [2.0]
optf = OptimizationFunction(f_root)
prob_bracket = OptimizationProblem(optf, x0_root, nothing, lb = [2.0], ub = [3.0])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a better fit to NonlinearSolve.jl for IntervalRootfindingProblem

@ChrisRackauckas
Copy link
Member

This seems rather complete! I think it would be better for the NonlinearProblem, NonlinearLeastSquaresProblem, and IntervalNonlinearProblem methods to live in NonlinearSolve.jl since the solvers for those types of problems are all associated there. NonlinearLeastSquaresProblem is a bit of an odd one, since all of the solvers are just NonlinearProblem methods, even though it's actually an optimization, but it goes together based on how the solvers act. How much code duplication would it be to have those split out?

Other than that, I setup the CI. If it's passing then I think things are looking pretty good.

@ChrisRackauckas
Copy link
Member

It looks like the Scipy install is having issues. Double check that's the right way to do the Python import with PythonCall.

@AdityaPandeyCN
Copy link
Contributor Author

I think we need to add a CondaPkg.toml with SciPy as dependency, your suggestions? @ChrisRackauckas

@ChrisRackauckas
Copy link
Member

That sounds right

Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
@AdityaPandeyCN
Copy link
Contributor Author

How much code duplication would it be to have those split out?

It will be around 150 lines of code with my analysis

@ChrisRackauckas
Copy link
Member

Ehh that is not so much. I think it would be good to split that over.

@ChrisRackauckas
Copy link
Member

Tests pass! So let's just split the NonlinearSolve parts and this is probably done.

Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
@ChrisRackauckas
Copy link
Member

Oh and the docs need a page for this.

@AdityaPandeyCN
Copy link
Contributor Author

Should I do that in a separate PR(docs)?

@ChrisRackauckas
Copy link
Member

Yeah that would be fine. Tests pass so merging.

@ChrisRackauckas ChrisRackauckas merged commit bc3c174 into SciML:master Jun 20, 2025
21 of 27 checks passed
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.

2 participants