Skip to content
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

Move solve to OptimalControl.jl #175

Closed
ocots opened this issue Jul 14, 2024 · 6 comments
Closed

Move solve to OptimalControl.jl #175

ocots opened this issue Jul 14, 2024 · 6 comments
Assignees

Comments

@ocots
Copy link
Member

ocots commented Jul 14, 2024

I propose to move the solve function to OptimalControl.jl.

We have to merge

function CommonSolve.solve(ocp::OptimalControlModel,

with

https://github.com/control-toolbox/OptimalControl.jl/blob/d139ce5cfc9c59be2e339a6045a5e1654fde2573/src/solve.jl#L67

Note that for the moment, even if in CTDirect.jl the solve function extends the one from CommonSolve.jl we still have the problem of qualifying for OptimalControl .jl. I think it would better to have only one common solve for an OptimalControlModel. In CTDirect.jl, we can keep the solve of a DOCP.

@PierreMartinon
Copy link
Member

PierreMartinon commented Jul 15, 2024

I agree that we should have the top-level solve in OptimalControl now.
I'll see how I organise the testing at CTDirect level.
The DOCP method can indeed stay in CTDirect, by the way, we could rename it as solve_DOCP ?
That would simplify the extension and would not interfere with the main solve exported in OptimalControl.

I'll start two branches and see how it goes.

@ocots
Copy link
Member Author

ocots commented Jul 15, 2024

If both solve methods extends the one from commonsolve it will not interfere i think. We can keep the name solve for the resolution of a docp. Thanks to multiple dispatxh it will be ok.

@PierreMartinon
Copy link
Member

Ok, branches for the first draft are up.

  • I added a new test in OptimalControl for the time_grid option, and plan to add more tests from CTDirect later
  • Since we currently only use the direct method, I kept the same signature as the former solve in CTDirect. We will probably adjust this when more solving methods are added.
  • @ocots help needed for the CommonSolve extension, for whatever reason I don't seem to be able to get it right. Can we schedule a meeting to do it together ? (if you prefer you can do it directly on the branches and I'll catch up later).

@PierreMartinon
Copy link
Member

PierreMartinon commented Jul 15, 2024

Ok, I think I got the export for solve, now the tests seem to run fine without prefixing, for both the direct and indirect cases.

I probably misunderstood what you said about CommonSolve: in OptimalControl we don't have a package extension (with ext/ and weakdeps etc), we only import CommonSolve.solve, add a new method and export.

@PierreMartinon
Copy link
Member

PierreMartinon commented Jul 15, 2024

Another small improvement: there is now an error message when trying to call solve without 'using NLPModelsIpopt' beforehand

@ocots @jbcaillau These should fix the current problems with OptimalControl.solve
#176 and control-toolbox/OptimalControl.jl#233

@jbcaillau
Copy link
Member

🙏🏽 @PierreMartinon
IMG_3349

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

No branches or pull requests

4 participants