-
-
Notifications
You must be signed in to change notification settings - Fork 39
Add support for SciMLStructures #413
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
|
redirect to master |
6bb9f54 to
c881ce0
Compare
Benchmark ResultsClick to check benchmark results
|
c881ce0 to
bd1d2d3
Compare
Co-Authored-By: Claude Opus 4.5 <[email protected]>
… using fit_parameters Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
e2f3c57 to
8d3f281
Compare
Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
@ErikQQY @ChrisRackauckas Can you take a look at this? I think it should be good to go and after a new release of the package(s) it should also fix the tests in SciML/ModelingToolkit.jl#4154 |
| newy = [u[i:(i + M - 1)] for i in 1:M:(length(u) - M + 1)] | ||
| eval_sol = EvalSol(newy, mesh, cache) | ||
| return fun(eval_sol, p) | ||
| end |
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.
The original unflatten operation is really allocating a lot, maybe we can change to use iterators to construct immediate solution from one-dimensional state vectors, for example reshape state vectors and use eachcol iterator?
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.
Changed to that in d5f757c.
|
This generally looks pretty good.
Since all of the optimal control functionality is using OptimiationIpopt, just courious if this bug affect the current usage of OptimizaitonIpopt in BoundaryValueDiffEq.jl? |
I tested now again and I can't reproduce the error anymore, it was happening with the above examples a few weeks ago, but I didn't dig in at the time. I'll look more into OptimizationIpopt, but currently it seems to be working. |
Fix: #346
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
This PR together with SciML/ModelingToolkit.jl#4154 allows us to target the optimal control interface and the parameter fitting interface that is developed here from the MTK side by working with parameters as SciMLStructures (
MTKParametersin the case of MTK). The MTK PR adds support for the codegen side of theBVPFunctionand with that the following snippet runsI'm not sure where should we add the tests for this, but at least this seems to work with MTK codegen on a simple case.
As a small note, there is a bug in OptimizationIpopt's constraint jabcobian handling, so I used MadNLP instead.
Should I add more testing here or would that go on the MTK side? or both? Since this is a PR to a PR, it would be harder to run tests from the MTK side.
@ChrisRackauckas Let me know what you think and how should I proceed here.