SciML / SciMLBase.jl

The Base interface of the SciML ecosystem
https://docs.sciml.ai/SciMLBase/stable
MIT License
127 stars 92 forks source link

add stats to ensemble solution #512

Closed pepijndevos closed 9 months ago

pepijndevos commented 10 months ago

This is based on https://github.com/SciML/DiffEqBase.jl/pull/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 commented 10 months ago

Rebase?

pepijndevos commented 10 months ago

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

pepijndevos commented 10 months ago

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

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

pepijndevos commented 10 months ago

Made a start: https://github.com/SciML/DiffEqBase.jl/pull/948 https://github.com/SciML/OrdinaryDiffEq.jl/pull/2038

codecov[bot] commented 10 months ago

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%) :arrow_up:
src/ensemble/ensemble_solutions.jl 44.95% <83.33%> (+19.95%) :arrow_up:
src/solutions/nonlinear_solutions.jl 50.00% <0.00%> (-40.91%) :arrow_down:
ext/SciMLBaseZygoteExt.jl 35.71% <0.00%> (+35.71%) :arrow_up:
src/solutions/ode_solutions.jl 81.48% <23.52%> (+6.05%) :arrow_up:

... and 23 files with indirect coverage changes

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

ChrisRackauckas commented 10 months ago

Needs to import printf

oscardssmith commented 10 months ago

counterpoint: just use print

pepijndevos commented 10 months ago

Derp sorry

ChrisRackauckas commented 10 months ago

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

pepijndevos commented 10 months ago

Is there an easy way to run some relevant set of tests to avoid a bunch more round trips like this :see_no_evil: 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 commented 10 months ago

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 commented 10 months ago

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

  1. add DEStats here
  2. add the forward in DiffEqBase
  3. 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 commented 10 months ago

@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 commented 10 months ago

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

pepijndevos commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

staticfloat commented 9 months ago

@ChrisRackauckas Is this good to merge now?

ChrisRackauckas commented 9 months ago

No there's still ensemble tests failing from this.

pepijndevos commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

Hurray tysm!