-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
Better BPINN ode Solver #853
Conversation
@ChrisRackauckas @Vaibhavdixit02 for the new formulations, merging of this PR first and PR #842 later is recommended. |
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.
LGTM
test/bpinnexperimental.jl
Outdated
@@ -0,0 +1,140 @@ | |||
using Test, MCMCChains |
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 new test file is not actually ran.
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, fixed
@test abs(param1 - p) < abs(0.3 * p) | ||
|
||
#-------------------------- solve() call | ||
# (lux chain) | ||
@test mean(abs.(physsol2 .- sol3lux_pestim.ensemblesol[1])) < 0.15 | ||
@test mean(abs.(physsol2 .- pmean(sol3lux_pestim.ensemblesol[1]))) < 0.15 |
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.
pmean
typo?
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.
nope, the mean is required as the solution's standard deviation are different at domain points, sometimes these uncertainties can be large enough for the tests to fail. so i just take the means for testing.
src/collocated_estim.jl
Outdated
@@ -0,0 +1,46 @@ | |||
# suggested extra loss function for ODE solver case | |||
function L2loss2(Tar::LogTargetDensity, θ) |
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.
why is this a separate file/
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.
fixed
My understanding is that the proposed changes from the last review were not implemented and this was just rebased? Is there a plan to finish this? |
This is ready @ChrisRackauckas the other test failures seem unrelated. Let me know if changes are needed. |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.