-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Scipywrapper #927
Conversation
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
…nd bug fixes Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
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") |
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.
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}; |
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.
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} |
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 is for NonlinearSolve.jl instead?
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.
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) |
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 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>
I have tried to address the suggestions @ChrisRackauckas |
@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]) |
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 is a better fit to NonlinearSolve.jl for IntervalRootfindingProblem
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. |
It looks like the Scipy install is having issues. Double check that's the right way to do the Python import with PythonCall. |
I think we need to add a CondaPkg.toml with SciPy as dependency, your suggestions? @ChrisRackauckas |
That sounds right |
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
It will be around 150 lines of code with my analysis |
Ehh that is not so much. I think it would be good to split that over. |
Tests pass! So let's just split the NonlinearSolve parts and this is probably done. |
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
Oh and the docs need a page for this. |
Should I do that in a separate PR(docs)? |
Yeah that would be fine. Tests pass so merging. |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
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:
I still think I can make it more better but reviews from the maintainers will take it in the correct direction.