add stats to ensemble solution #512

pepijndevos · 2023-09-29T08:54:52Z

This is based on SciML/DiffEqBase.jl#938

Example

using OrdinaryDiffEq
f(u,p,t) = 1.01*u
u0=1/2
tspan = (0.0,1.0)
prob = ODEProblem(f,u0,tspan)
function prob_func(prob, i, repeat)
    remake(prob, u0 = rand() * prob.u0)
end
ensemble_prob = EnsembleProblem(prob, prob_func = prob_func)
sim = solve(ensemble_prob, Tsit5(), EnsembleThreads(), trajectories = 10)
using Plots;
plot(sim)
sim.stats

DiffEqBase.Stats
Number of function 1 evaluations:                  270
Number of function 2 evaluations:                  0
Number of W matrix evaluations:                    0
Number of linear solves:                           0
Number of Jacobians created:                       0
Number of nonlinear solver iterations:             0
Number of nonlinear solver convergence failures:   0
Number of rootfind condition calls:                0
Number of accepted steps:                          40
Number of rejected steps:                          0

ChrisRackauckas · 2023-09-29T14:48:30Z

src/ensemble/basic_ensemble_solve.jl

+function merge_stats(us)
+    mapreduce(x -> x.stats, merge, us)


This is not safe for if stats is nothing

Also, this merge is not defined for most stats types?

Oh no... there are more stats types? Well of course there are. Tbh the hardest part of adding Ensemble statistics is figuring out where all the stuff lives. Do you have a better idea to provide Ensemble stats, or do I just need to hunt down every stats object in the SciML ecosystem and implement a merge method for it? Or can I just handle the nothing case and catch the case where merge isn't defined and if some obscure type of system wants Ensemble stats they can go and add it?

I think what we should do is just move the diffeq stats object to SciMLBase and then add the merge function here. That would put it more inline with how the other bits are developing, like https://github.com/SciML/SciMLBase.jl/blob/master/src/solutions/nonlinear_solutions.jl#L2-L18. Since it doesn't require downstream functionality and is instead part of the general interface, it's a bit weird that Stats (which should be DEStats) is not in SciMLBase but is instead in DiffEqBase. That's just an organizational mistake. Can you make that swap as part of this PR?

That seems quite a delicate dance of breaking changes and version bumps/pins all over SciML that I'm not sure I'm the right person to make.

https://github.com/search?q=org%3ASciML+%22DiffEqBase.Stats%22&type=code

Handling the nothing case and method error is done though.

ChrisRackauckas · 2023-10-08T10:25:04Z

Rebase?

pepijndevos · 2023-10-08T13:22:22Z

darn I thought I could quickly do it from the web interface but it did a merge. Something for tomorrow I guess.

pepijndevos · 2023-10-09T08:46:53Z

It occurred to me that the move might be simpler than I thought.

add it here
mark original deprecated

From there packages can slowly transition, no need for me to try to transition the entire ecosystem in one go

pepijndevos · 2023-10-09T09:28:11Z

Made a start:
SciML/DiffEqBase.jl#948
SciML/OrdinaryDiffEq.jl#2038

codecov · 2023-10-10T13:00:21Z

Codecov Report

Merging #512 (35f5f06) into master (7afe983) will increase coverage by 7.71%.
Report is 30 commits behind head on master.
The diff coverage is 32.69%.

@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
+ Coverage   47.36%   55.07%   +7.71%     
==========================================
  Files          53       54       +1     
  Lines        3921     3980      +59     
==========================================
+ Hits         1857     2192     +335     
+ Misses       2064     1788     -276

Files	Coverage Δ
src/SciMLBase.jl	`78.26% <ø> (ø)`
src/ensemble/basic_ensemble_solve.jl	`80.35% <100.00%> (+27.52%)`	⬆️
src/ensemble/ensemble_solutions.jl	`44.95% <83.33%> (+19.95%)`	⬆️
src/solutions/nonlinear_solutions.jl	`50.00% <0.00%> (-40.91%)`	⬇️
ext/SciMLBaseZygoteExt.jl	`35.71% <0.00%> (+35.71%)`	⬆️
src/solutions/ode_solutions.jl	`81.48% <23.52%> (+6.05%)`	⬆️

... and 23 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/ensemble/basic_ensemble_solve.jl

ChrisRackauckas · 2023-10-10T14:00:56Z

Needs to import printf

oscardssmith · 2023-10-10T14:04:13Z

counterpoint: just use print

pepijndevos · 2023-10-10T14:32:04Z

Derp sorry

ChrisRackauckas · 2023-10-11T04:27:15Z

looks like reduction options aren't handled: https://github.com/SciML/SciMLBase.jl/actions/runs/6477667738/job/17588215082?pr=512#step:6:989

pepijndevos · 2023-10-11T07:38:19Z

Is there an easy way to run some relevant set of tests to avoid a bunch more round trips like this 🙈
I see that master also fails some of the 60-something tests.
Looking at the code I guess I somehow need to set the GROUP environment variable to at least run the downstream tests.

pepijndevos · 2023-10-11T08:20:53Z

I think I managed to run tests with just doing ENV["GROUP"]="Downstream" but obviously it fails merging DiffEqBase.Stats until we do that change. So bit of a chicken-egg problem there where we need to have SciMLBase.DEStats to do the forward and we need the forward not to break things.

I'm not sure if I'm totally happy with how I handled reductions now. It makes sense that if you're doing custom reduction there isn't much to merge. Another option would be to just try and fail. Another option would be to try and do the check at the type level so the compiler has a better chance to figure out type stability.

pepijndevos · 2023-10-11T08:22:56Z

I guess if we want to do it absolutely breakage-free I should split up the move into a separate PR, so we can do

add DEStats here
add the forward in DiffEqBase
add the merge & stats here

[edit] oh nvm I see you handled that in https://github.com/SciML/DiffEqBase.jl/pull/949/files#diff-4707ffacbf76401dc918e8bfac5bf57023ff087628a2b6d2b741f943d972fa1cR1

ChrisRackauckas · 2023-10-16T09:20:31Z

@oscardssmith did this happen because of one of your recent changes? Can you investigate this breakage? https://github.com/SciML/SciMLBase.jl/actions/runs/6479781338/job/17731840088?pr=512#step:6:1003

ChrisRackauckas · 2023-10-16T16:08:12Z

Looks like there's still downstream breakage: https://github.com/SciML/SciMLBase.jl/actions/runs/6531788842/job/17734231446?pr=512#step:6:1470

pepijndevos · 2023-10-16T16:52:58Z

I'd still like to know how I can run a useful set of test locally because I did run the "Downstream" testset which didn't catch this. I'll have a closer look tomorrow what is causing this, yet another case of custom output function it seems. Will this ever end haha

ChrisRackauckas · 2023-10-16T16:54:01Z

It should just be the same as running the downstream tests? No different locally, nothing crazy. But you maybe had some extra things dev'd that fixed it.

pepijndevos · 2023-10-18T17:00:42Z

Turns out it was the Downstream tests of DiffEqBase that failed while I was running the Downstream tests of SciMLBase only.

staticfloat · 2023-10-27T18:27:35Z

@ChrisRackauckas Is this good to merge now?

ChrisRackauckas · 2023-10-28T03:28:20Z

No there's still ensemble tests failing from this.

pepijndevos · 2023-10-28T06:58:26Z

It's completely opaque to me which of these CI things are actually relevant and what they mean. I think I saw some errors about enzyme complaining about if statements and a random one I pressed just now complained about a missing adjoint. Feels like I need a math PhD and grok the entire sciml ecosystem to add a field to a struct.

ChrisRackauckas · 2023-10-28T08:11:50Z

This change is at a very low level and is effecting an interface used by tens of thousands of people for many diverse reasons. It should be taken seriously. Uniformity in interfaces requires that interface changes are thought through with a global view. Otherwise the ODEs won't work the same as the nonlinear solvers and the jump problems and then SciML loses its ease of use. So yes, changing the core interface isn't and shouldn't be easy to do. For this case, we can't just merge when tests are saying automatic differentiation is failing. Ask for help if you need it, I'm always willing to help, but we're not going to push something through that breaks AD

pepijndevos · 2023-10-28T08:15:30Z

Sorry yes it makes sense it's just hard to make sense of the CI output as a relative outsider to much of sciml.

pepijndevos · 2023-10-28T11:27:08Z

Hurray tysm!

pepijndevos mentioned this pull request Sep 29, 2023

merge stats objects SciML/DiffEqBase.jl#938

Closed

oscardssmith approved these changes Sep 29, 2023

View reviewed changes

ChrisRackauckas reviewed Sep 29, 2023

View reviewed changes

Pepijn de Vos added 4 commits October 9, 2023 11:15

add stats to ensemble solution

2b02160

add stats test

947695f

handle different stats types, including nothing

71c6527

move stats here as DEStats

fe2f12b

pepijndevos force-pushed the pv/enstats branch from d1c0a8d to fe2f12b Compare October 9, 2023 09:15

add show and merge to NLStats

c933d7f

pepijndevos mentioned this pull request Oct 9, 2023

use SciMLBase.DEStats SciML/OrdinaryDiffEq.jl#2038

Merged

pepijndevos mentioned this pull request Oct 9, 2023

Use SciMLBase.DEStats SciML/Sundials.jl#429

Merged

ChrisRackauckas reviewed Oct 10, 2023

View reviewed changes

src/ensemble/basic_ensemble_solve.jl Outdated Show resolved Hide resolved

Update src/ensemble/basic_ensemble_solve.jl

d05c8a3

ChrisRackauckas approved these changes Oct 10, 2023

View reviewed changes

add printf import

365530b

pepijndevos mentioned this pull request Oct 10, 2023

deprecate stats SciML/DiffEqBase.jl#948

Closed

ChrisRackauckas closed this Oct 11, 2023

ChrisRackauckas reopened this Oct 11, 2023

only merge stats on default reduction

7c0b301

backwards compat for stats argument

45efdc5

try to fix tests

ae30704

untyped stats

a44c420

ChrisRackauckas added 2 commits October 28, 2023 09:08

Update SciMLBaseZygoteExt.jl

9cb8c14

Update SciMLBaseZygoteExt.jl

30c0342

Update SciMLBaseZygoteExt.jl

35f5f06

ChrisRackauckas merged commit ff42d8b into SciML:master Oct 28, 2023
56 of 70 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add stats to ensemble solution #512

add stats to ensemble solution #512

pepijndevos commented Sep 29, 2023 •

edited

Loading

ChrisRackauckas Sep 29, 2023

ChrisRackauckas Sep 29, 2023

pepijndevos Sep 29, 2023

ChrisRackauckas Sep 30, 2023

pepijndevos Oct 2, 2023

ChrisRackauckas commented Oct 8, 2023

pepijndevos commented Oct 8, 2023

pepijndevos commented Oct 9, 2023 •

edited

Loading

pepijndevos commented Oct 9, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading

ChrisRackauckas commented Oct 10, 2023

oscardssmith commented Oct 10, 2023

pepijndevos commented Oct 10, 2023

ChrisRackauckas commented Oct 11, 2023

pepijndevos commented Oct 11, 2023

pepijndevos commented Oct 11, 2023

pepijndevos commented Oct 11, 2023 •

edited

Loading

ChrisRackauckas commented Oct 16, 2023

ChrisRackauckas commented Oct 16, 2023

pepijndevos commented Oct 16, 2023

ChrisRackauckas commented Oct 16, 2023

pepijndevos commented Oct 18, 2023

staticfloat commented Oct 27, 2023

ChrisRackauckas commented Oct 28, 2023

pepijndevos commented Oct 28, 2023

ChrisRackauckas commented Oct 28, 2023

pepijndevos commented Oct 28, 2023

pepijndevos commented Oct 28, 2023

add stats to ensemble solution #512

add stats to ensemble solution #512

Conversation

pepijndevos commented Sep 29, 2023 • edited Loading

ChrisRackauckas Sep 29, 2023

Choose a reason for hiding this comment

ChrisRackauckas Sep 29, 2023

Choose a reason for hiding this comment

pepijndevos Sep 29, 2023

Choose a reason for hiding this comment

ChrisRackauckas Sep 30, 2023

Choose a reason for hiding this comment

pepijndevos Oct 2, 2023

Choose a reason for hiding this comment

ChrisRackauckas commented Oct 8, 2023

pepijndevos commented Oct 8, 2023

pepijndevos commented Oct 9, 2023 • edited Loading

pepijndevos commented Oct 9, 2023

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

ChrisRackauckas commented Oct 10, 2023

oscardssmith commented Oct 10, 2023

pepijndevos commented Oct 10, 2023

ChrisRackauckas commented Oct 11, 2023

pepijndevos commented Oct 11, 2023

pepijndevos commented Oct 11, 2023

pepijndevos commented Oct 11, 2023 • edited Loading

ChrisRackauckas commented Oct 16, 2023

ChrisRackauckas commented Oct 16, 2023

pepijndevos commented Oct 16, 2023

ChrisRackauckas commented Oct 16, 2023

pepijndevos commented Oct 18, 2023

staticfloat commented Oct 27, 2023

ChrisRackauckas commented Oct 28, 2023

pepijndevos commented Oct 28, 2023

ChrisRackauckas commented Oct 28, 2023

pepijndevos commented Oct 28, 2023

pepijndevos commented Oct 28, 2023

pepijndevos commented Sep 29, 2023 •

edited

Loading

pepijndevos commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

pepijndevos commented Oct 11, 2023 •

edited

Loading