-
-
Notifications
You must be signed in to change notification settings - Fork 114
Add BVPFunction #370
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
Add BVPFunction #370
Conversation
Signed-off-by: ErikQQY <[email protected]>
|
I'm going to hold onto this: I'm not quite convinced that BVPs need a different function since they are just ODEs with extra boundary conditions. We are going to do a BVP overhaul quite soon though, so we'll evaluate it as we start making new BVP functionality and see what extra fields we could potentially use. |
|
Sure, I will follow the BVP overhaul and keep this PR as a record in case a BVPFunction is needed. |
Codecov Report
@@ Coverage Diff @@
## master #370 +/- ##
==========================================
+ Coverage 53.73% 55.17% +1.43%
==========================================
Files 47 50 +3
Lines 3584 3701 +117
==========================================
+ Hits 1926 2042 +116
- Misses 1658 1659 +1
... and 17 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: ErikQQY <[email protected]>
|
I added |
Signed-off-by: ErikQQY <[email protected]>
Signed-off-by: ErikQQY <[email protected]>
src/problems/bvp_problems.jl
Outdated
|
|
||
| function BVProblem(f, bc, u0, tspan, p = NullParameters(); kwargs...) | ||
| BVProblem(ODEFunction(f), bc, u0, tspan, p; kwargs...) | ||
| BVProblem(BVPFunction(f, bc), bc, u0, tspan, p; kwargs...) |
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.
If BVPFunction stores bc should be duplicate the storage in BVProblem again?
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.
Yeah, indeed, the problem construction process in BVProblem is similar with SDEProblem, they both have duplicate bc in BVProblem and g in SDEProblem, but if we want to have a BVPFunction stores both f and bc, duplicating bc could really making the problem constructor concise. We need to note that when we are unpacking BVProblem, we actually get a BVPFunction but not f.
SciMLBase.jl/src/problems/sde_problems.jl
Lines 130 to 132 in 4d926fc
| function SDEProblem(f, g, u0, tspan, p = NullParameters(); kwargs...) | |
| SDEProblem(SDEFunction(f, g), g, u0, tspan, p; kwargs...) | |
| 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.
In that case it's fair to stay consistent.
Maybe we can add an additional dispatch on BVPFunction where we automatically pull out the bc during problem construction that way user wont have to do BVProblem(BVPFunction(f, bc), bc...) and instead can specify BVProblem(BVPFunction(f, bc)....)
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.
I think you mean a dispatch on BVProblem right? Just updated and now we can directly specify BVProblem(BVPFunction(f, bc).....) to construct a BVProblem.
Do we need to do the same for SDEProblem and SDEFunction? The problem construct of SDEProblem also need users to do somthing like SDEProblem(SDEFunction(f, g), g), see here: https://docs.sciml.ai/DiffEqDocs/stable/tutorials/sde_example/#Using-Higher-Order-Methods
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.
@ChrisRackauckas do you think this is a valid API choice? Is there a particular reason other Problem Types don't have this?
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.
can you handle downstream?
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.
Sure, I am working on fixing downstream errors
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.
I am a little lost here, there are three ways of constructing a BVProblem:
(1). Directly construction from scratch
prob = BVProblem(f, bc, u0, tspan)(2). Use BVPFunction
prob = BVProblem(BVPFunction(f, bc), u0, tspan)(3). Another way of using BVPFunction
prob = BVProblem(BVPFunction(f, bc), bc, u0, tspan)As for SDEProblem, the definitions are similar, so my question is that it looks the (2) and (3) dispatches can't exist at the same time, so I think we are deprecating (3) and using (2) 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.
yes
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.
It looks like deprecating (3) and using (2) in SDEProblem and SDEFunction is way more complicated than BVProblem and BVPFunction. SplitSDEProblem, DynamicalSDEProblem and maybe some functions in ModelingToolkit.jl and JumpProcess.jl etc. are all relying on (3) in problem constructor, the new change in this PR would cause a lot of errors and break a lot of APIs.
Signed-off-by: ErikQQY <[email protected]>
Signed-off-by: ErikQQY <[email protected]>
src/problems/sde_problems.jl
Outdated
| end | ||
|
|
||
| function SDEProblem(f::AbstractSDEFunction, u0, tspan, p = NullParameters(); kwargs...) | ||
| SDEProblem(f, f.g, u0, tspan, p; kwargs...) |
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 fallback won't work since it will drop information like Jacobian types.
Signed-off-by: ErikQQY <[email protected]>
Signed-off-by: ErikQQY <[email protected]>
|
I am still a little concerned the breaking changes would affect a lot, as can be seen from the CI status, the API changes especially |
|
If the SDE thing requires a lot of patching, can we decouple it from the BVPFunction work? The BVP function is non-breaking IIUC. |
Yeah agreed, I think we can first go with the BVP stuff, then update the SDE things in another PR. |
Signed-off-by: ErikQQY <[email protected]>


This PR adds
BVPFunction. I think it would be nice for BoundaryValueDiffEq.jl to have this feature, since we have all kinds of*Functionfor all kinds of problems.It is very interesting that after I added the
BVPFunctionfeature to SciMLBase.jl and used the simple pendulum example to test if theBVPFunctioncan be directly used to solve the problem, and tada!🎉