-
-
Notifications
You must be signed in to change notification settings - Fork 101
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
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
BVProblem{isinplace(f, 4)}(f, bc, u0, tspan, args...; kwargs...) | ||
end | ||
|
||
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, g, u0, tspan, p = NullParameters(); kwargs...) | ||
SDEProblem(SDEFunction(f, g), g, u0, tspan, p; kwargs...) | ||
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*Function
for all kinds of problems.It is very interesting that after I added the
BVPFunction
feature to SciMLBase.jl and used the simple pendulum example to test if theBVPFunction
can be directly used to solve the problem, and tada!🎉