feat: continuous-time reservoirs via RCODEReservoirExt#450
Conversation
Implements the continuous `_collectstates` and `predict` overrides for `SciMLProblemReservoir` via the OrdinaryDiffEq + SciMLBase + DataInterpolations package extension, building on the core types landed in SciML#446. The extension: - Interpolates input columns onto a per-window grid through a custom `LinearInputInterp` struct (avoids a DataInterpolations matrix-`u` + SciMLBase dual-eltype probing interaction). - Aligns `saveat` so `states[:, k]` is the reservoir state after processing input column `k` — inputs at window starts, samples at window ends. This is what makes the Euler-equivalence test land without an off-by-one shift. - Threads `states_modifiers` per saved sample. - Ships teacher-forced `predict` (one bulk solve + readout per column) and autoregressive `predict` (per-step sub-solves with the previous readout output held constant on each window). - Errors loudly on `:input` collision in `prob.p` / `ps.reservoir`, on degenerate `tspan`, and on non-NamedTuple `prob.p`. Reservoir construction now rejects the three protected solve kwargs (`saveat`, `save_everystep`, `dense`) so they cannot reach the solver and silently desync the sample grid. `src/predict.jl` gains the two-level `predict` -> `_predict` dispatch mirroring the `collectstates` pattern from PR1; the in-`src/` stubs error with a clear message when the extension is not loaded. Tests: 34 extension tests covering Euler equivalence, linear-ODE analytic match, sampler shape, both predict modes (with determinism), state modifiers on the continuous path, T_steps / steps boundary errors, reserved-key collision, `tspan` validation, and the three accepted `prob.p` shapes (NamedTuple, `nothing`, `NullParameters`). `test_sciml_reservoir.jl` extended with protected-kwarg and predict-error checks. Full Core suite (1413 tests) passes locally; Aqua / ExplicitImports / JET / Runic all clean.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an ODE-backed SciMLProblemReservoir implementation via a package extension, with validation and tests to ensure correct sampling/prediction behavior and clearer failure modes when the extension isn’t loaded.
Changes:
- Introduces
RCODEReservoirExtimplementing continuous_collectstatesand_predictforSciMLProblemReservoir. - Adds constructor-time validation for protected
solvekwargs and improves extension-required error messaging. - Adds a dedicated ODE extension test suite and wires it into
runtests.jl; updatesProject.tomlweakdeps/extensions accordingly.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_sciml_reservoir.jl | Adds negative tests for protected kwargs and missing-extension predict errors. |
| test/test_ode_reservoir_ext.jl | New end-to-end tests for continuous collectstates/predict semantics, shapes, and validation. |
| test/runtests.jl | Includes the new ODE reservoir extension testset. |
| src/predict.jl | Adds _predict dispatch split for continuous vs discrete reservoirs and fixes a docstring typo. |
| src/layers/sciml_reservoir.jl | Documents sampling semantics and rejects protected solve kwargs at construction. |
| ext/RCODEReservoirExt.jl | New extension implementing continuous collectstates and predict for SciML reservoirs. |
| Project.toml | Registers extension dependencies (weakdeps + compat + test targets). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct LinearInputInterp{D <: AbstractMatrix, T <: AbstractVector} | ||
| data::D | ||
| ts::T | ||
| end | ||
|
|
||
| function (interp::LinearInputInterp)(t) | ||
| ts = interp.ts | ||
| n = length(ts) | ||
| if t ≤ ts[1] | ||
| return interp.data[:, 1] | ||
| elseif t ≥ ts[end] | ||
| return interp.data[:, n] | ||
| end | ||
| k = searchsortedlast(ts, t) | ||
| k = clamp(k, 1, n - 1) | ||
| α = (t - ts[k]) / (ts[k + 1] - ts[k]) | ||
| return (1 - α) .* interp.data[:, k] .+ α .* interp.data[:, k + 1] | ||
| end |
| Δt = (t1 - t0) / T_steps | ||
| input_ts = collect(range(t0, t1 - Δt; length = T_steps)) | ||
| sample_ts = collect(range(t0 + Δt, t1; length = T_steps)) | ||
|
|
||
| input_interp = _make_input_fn(data, input_ts) | ||
| solve_p = _build_solve_params(res.prob.p, ps.reservoir, input_interp) |
| ts = collect(range(t0, t1; length = steps + 1)) | ||
|
|
||
| x_current = collect(res.prob.u0) | ||
| current_input = collect(initialdata) | ||
|
|
||
| out_dim = length(initialdata) | ||
| output = Matrix{eltype(initialdata)}(undef, out_dim, steps) |
| function _predict(::Any, rc::AbstractReservoirComputer, data::AbstractMatrix, ps, st) | ||
| T = size(data, 2) | ||
| @assert T ≥ 1 "data must have at least one time step (columns)." | ||
|
|
| output = zeros(eltype(initialdata), length(initialdata), steps) | ||
| for step in 1:steps | ||
| initialdata, st = apply(rc, initialdata, ps, st) | ||
| output[:, step] = initialdata | ||
| end | ||
| return output, st |
| out_dim = length(initialdata) | ||
| output = Matrix{eltype(initialdata)}(undef, out_dim, steps) |
| function rhs_noparams!(dx, x, p, t) | ||
| u = p.input(t) | ||
| return dx .= .-x .+ Win_noparams * u | ||
| end | ||
| global Win_noparams = 0.5 .* randn(rng, res_dim, in_dim) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 543723b901
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| ts = collect(range(t0, t1; length = steps + 1)) | ||
|
|
||
| x_current = collect(res.prob.u0) |
There was a problem hiding this comment.
Handle scalar ODE states in autoregressive rollout
For a valid one-dimensional ODEProblem whose u0 is a scalar (for example u0 = 0.0), the new autoregressive path fails before the first solve because collect(::Number) is not defined. collectstates can still sample scalar SciML solutions into a 1-row state matrix, so this makes predict(rc, steps, ...) unusable for the same scalar reservoirs unless users artificially wrap the state in a vector.
Useful? React with 👍 / 👎.
Apply Copilot review feedback on SciML#450: - Switch the input signal from linear interpolation to zero-order hold (`ZeroOrderHoldInterp`). Linear interp made `states[:, k]` depend on `data[:, k+1]` for any non-Euler solver because the signal ramps toward the next column across the window. ZOH ensures only `data[:, k]` influences `states[:, k]` regardless of solver, and the call-site returns a `view` so the input function is allocation-free in the ODE hot path. The autoregressive `predict` path already used ZOH, so the two paths now use the same scheme. - Stop wrapping `res.prob.u0` and `initialdata` in `collect(…)` inside the autoregressive `predict`. `collect` either errored (no method on `Number`) or destroyed structured state types like `SVector` / `ComponentArray`. The loop only reads these values, never mutates them in place, so a direct reference is safe. - Allocate the autoregressive `output` matrix from the first readout output via `similar(y, length(y), steps)` rather than `Matrix{eltype(initialdata)}(undef, …)`. Otherwise a readout returning a different eltype (e.g. Float64 from a Float32 input) would force a silent conversion at every column assignment. - Replace `@assert T ≥ 1` with an `ArgumentError` in the new `_predict(::Any, rc::AbstractReservoirComputer, data, …)` fallback so the continuous-path API uses one error type for size validation. Pre-existing `predict(::AbstractLuxLayer, …)` left untouched. - Drop the `global Win_noparams` in the `prob.p` acceptance test in favour of a `let`-block closure, avoiding the type-instability and cross-test leakage hazards of a module-scope binding. Tests still 34/34 in the extension suite, 1413/1413 in the full Core suite; Aqua / ExplicitImports / JET / Runic all clean.
|
Pushed Fixed
Deferred (with reasoning)
Local suite still green: 34/34 extension, 1413/1413 Core, Aqua 10/10, ExplicitImports 2/2, JET clean, Runic clean. |
The docs project pins `OrdinaryDiffEqAdamsBashforthMoulton = "2"`, but OrdinaryDiffEq v7 requires that subpackage at v3. Allowing `"6, 7"` made docs Pkg.resolve fail on the GPU Documentation CI runner because the solver had no compatible AdamsBashforthMoulton version left. OrdinaryDiffEq v6 (the line our local Core suite verifies against) plays nice with AdamsBashforthMoulton v2, so the extension keeps working end-to-end. v7 support can be added in a follow-up that also bumps `docs/Project.toml`.
After narrowing `OrdinaryDiffEq` to v6, the docs build hit the next unsatisfiable: `docs/Project.toml` pins `OrdinaryDiffEqAdamsBashforthMoulton = "2"`, and that subpackage at v2 requires SciMLBase v3, but our compat was capped at v2. The extension only uses `remake`, `solve`, and `NullParameters` from SciMLBase — all stable across the v2 → v3 boundary — so widening to `"2, 3"` is safe and lets docs resolve. Local Core suite was verified on SciMLBase v2; Core CI matrix is unaffected because nothing in `[targets].test` forces v3.
Three independent issues were preventing the docs build: - The new `predict(rc::AbstractReservoirComputer, …)` dispatch reached for `rc.reservoir` unconditionally, but `DeepESN <: AbstractReservoirComputer` carries its leading field as `:cells`, not `:reservoir`. The docs `@example` block in `tutorials/deep_esn.md` therefore crashed with `FieldError`. Add a `hasfield(typeof(rc), :reservoir)` guard so subtypes without a reservoir field fall through to the `::Any` `_predict` fallback (which just routes through `apply(rc, …)` — DeepESN already implements this). A `test_sciml_reservoir.jl` testset now exercises both `predict` signatures on `DeepESN` so this can't regress silently. - Documenter's `:missing_docs` check flagged the four public types introduced in SciML#446 (`SciMLProblemReservoir`, `AbstractSciMLProblemReservoir`, `AbstractSampler`, `TerminalStateSampling`) as having docstrings but no manual entry. Add a "Continuous-Time Reservoirs" section to `docs/src/api/layers.md` with the matching `@docs` block. - `_PROTECTED_SOLVE_KWARGS` was an internal helper that picked up a `"""` docstring during the PR1 review pass and that tripped `:missing_docs` too. Convert it to a regular `#` comment so Documenter does not pick it up. Full Core suite (1417 tests including the new DeepESN regression testset) green; Runic clean.
…e CI `SciMLBase = "2, 3"` + `PrecompileTools = "1"` resolved to SciMLBase 2.0.0 and PrecompileTools 1.0.0 under Pkg's Downgrade strategy. SciMLBase ≤ 2.10 is the only range PrecompileTools 1.0.0 admits, but the test extra DifferentialEquations 7.16.1 requires SciMLBase ≥ 2.51 — empty intersection, unsatisfiable. Tighten both lower bounds so the Downgrade resolve succeeds.
| n = length(ts) | ||
| t < ts[1] && return view(interp.data, :, 1) | ||
| t ≥ ts[end] && return view(interp.data, :, n) | ||
| k = searchsortedlast(ts, t) |
There was a problem hiding this comment.
where possible avoid single letter variables
There was a problem hiding this comment.
this is valid for the whole pr
| # not be silently truncated back to the reservoir state's eltype. | ||
| out = similar(col1, length(col1), T) | ||
| out[:, 1] .= col1 | ||
| for t in 2:T |
There was a problem hiding this comment.
is there a way to use built-in functions like eachcol? if so that would be preferred here
There was a problem hiding this comment.
this is valid for most for loops
|
beside a couple of comments this looks good to me. I think that having a docs page would also help the user, I forgot if this is a later deliverable or if it should go in here |
- Replace single-letter runtime variables across the PR (n, k, T, t, y, Y, yt, y1, b, x, out, col1) with descriptive names. Keep SciML/ODE RHS conventions (dx, x, p, t, u) and one- to two-letter math symbols (t0, t1, Δt) because they are codebase-wide idioms. - Switch column-walking for-loops to `eachcol` + `Iterators.drop` + `enumerate` in `_apply_modifiers_continuous`, the teacher-forced `_predict`, and the discrete `_predict` fallback. The autoregressive predict iterates `zip(window_starts, window_ends)` so the per-window bounds carry their semantic name. - Add `docs/src/tutorials/sciml_reservoir.md` introducing the `SciMLProblemReservoir` API with a linear-ODE worked example, and wire it into the tutorials section of `docs/pages.jl`. Local: GROUP=Core 1417/1417 pass, GROUP=nopre 13/13 pass (Aqua + JET + ExplicitImports), Runic clean.
Bump from position 5 to position 3, right after the canonical ESN Lorenz example. Continuous-time reservoirs are a major API surface rather than a niche variant, so they should be reachable before NGRC and DeepESN.
`NNlib 0.9.26` only admits `ForwardDiff = "0.10.36-0.10"`. Under Downgrade, the new SciML weakdeps pull `LogExpFunctions 1.0.1` into the sandbox, and that LogExpFunctions version only admits ForwardDiff in `[0.9-0.10.22, 1.4.0]` — empty intersection with `[0.10.36-0.10.39]`. NNlib 0.9.30 widens its compat to also admit ForwardDiff 1.x; with the extra freedom, Resolver.jl --min picks LogExpFunctions 0.3.29 instead of 1.0.1, which has different ForwardDiff constraints and lets the sandbox resolve to ForwardDiff 0.10.39. Verified locally on Julia 1.10 by replaying the action's exact pipeline (`julia-actions/julia-downgrade-compat@v2`: merged-project + Resolver.jl --min + Pkg.test): all test groups pass (Common Utilities 78, Layers 457, Echo State Networks 817, NGRC 31, ODE Reservoir Extension 34).
… layers.md Per Francesco's note, the SciML reservoir section reads more naturally between Wrappers and the cellular-automata reservoir than tacked on at the bottom of the page.
|
everything looks good, after we fix the conflicts I think this is good to merge. Could you provide a quick eye test and reproduce the readme example with a continuous esn? just feeding the raw equations into the |
Resolve PR2 vs master conflicts after the SciMLTesting v1.2 migration landed on master (PR SciML#454, SciML#453, SciML#452, SciML#451, SciML#449): - Project.toml: keep our weakdeps (DataInterpolations, OrdinaryDiffEq, SciMLBase), our compat bumps (NNlib 0.9.30, PrecompileTools 1.2), and our test extras. Adopt master's SciMLTesting test dep, drop the Pkg test dep that master removed (the v1.2 harness owns Pkg ops). - test/runtests.jl: replace the hand-rolled SafeTestsets + GROUP dispatch with `using SciMLTesting; run_tests()` — folder-discovery picks up `test/test_ode_reservoir_ext.jl` and `test/test_sciml_reservoir.jl` automatically. Also follow up on Francesco's PR-wide style review: - Rename the remaining single-letter `u` / `u_val` bindings inside RHS bodies in ext + tests + the docs example to `input_t` for consistency with the rename pass already applied PR-wide. - Add a Lorenz chaos-forecasting eye-test section in the SciML reservoir tutorial that mirrors the README ESN example but feeds the raw leaky-integrator continuous ESN equations into a `SciMLProblemReservoir` (no CTESN struct yet, per Francesco's ask). Local replay (Julia 1.10, SciMLTesting v1.2 folder-discovery): GROUP=Core passes all groups including `Core/test_ode_reservoir_ext.jl` 34/34 and `Core/test_sciml_reservoir.jl` 24/24.
…uild The new Lorenz eye-test `@example` block in the SciML reservoir tutorial loads these three packages to fire the `RCODEReservoirExt` extension, but the docs build environment only had `OrdinaryDiffEqAdamsBashforthMoulton` available. Documentation CI failed with `Package OrdinaryDiffEq not found in current path`. Mirror the main package's compat bounds so the docs resolution lines up with the runtime resolution.
…Interpolations The previous attempt at unblocking the docs build by widening `OrdinaryDiffEq` compat to "6, 7" broke the test suite — OrdinaryDiffEq v7 dropped `Euler` from its top-level exports, so `using OrdinaryDiffEq: Euler` in the Euler-equivalence test fails on v7. Pinning back to "6" brought back the original docs build conflict (`OrdinaryDiffEqAdamsBashforthMoulton 2` requires `DiffEqBase 7`, which requires `OrdinaryDiffEq 7`). Architecturally cleaner fix: the extension never references any `OrdinaryDiffEq` symbol — it only uses `SciMLBase`'s `solve` / `remake` and dispatches via the concrete solver type the user passes in `res.args[1]`. So `OrdinaryDiffEq` was a redundant weakdep trigger. - Drop `OrdinaryDiffEq` from `[weakdeps]`; the extension trigger becomes `["SciMLBase", "DataInterpolations"]`. - Keep `OrdinaryDiffEq = "6"` in `[compat]` so the test sandbox still resolves it to v6 (where `Tsit5` and `Euler` are top-level exports); the test code is unchanged. - Drop the `using OrdinaryDiffEq: OrdinaryDiffEq` trigger import from the extension; refresh the comment to explain that the user picks any solver package separately. - Docs: use `OrdinaryDiffEqTsit5` (small subpackage) instead of full `OrdinaryDiffEq` for the Lorenz eye-test, avoiding the AB-M / DiffEqBase / OrdinaryDiffEq version cascade. Bump the Lorenz data tspan to `(0.0, 40.0)` so the existing `shift + train_len + predict_len = 1550` slice fits in the saveat grid (was failing at 1501 columns). - Update the error messages and the docstring on `AbstractSciMLProblemReservoir` / `SciMLProblemReservoir` so users know they need `SciMLBase` + `DataInterpolations` + a solver package of their choice, not specifically `OrdinaryDiffEq`. Local Julia 1.10: GROUP=Core 1417/1417 pass, including the ODE-extension and SciMLProblemReservoir testsets. Runic clean. Docs example runs end-to-end producing the expected (3, 250) prediction matrix.
Runic v1.7's stricter continuation rule wants the second line of the multi-line error string at the same indent as the other lines (12 sp, not 16). My local v1.7 missed it earlier; this matches what CI runic flagged.
New section "A delay-equation target: Mackey-Glass" in the SciMLProblemReservoir tutorial. Generates the canonical Mackey-Glass chaotic time series (τ = 17) as a DDEProblem solved with MethodOfSteps(Tsit5()), then forecasts it autoregressively with the same continuous ESN reservoir machinery used for the Lorenz example, retuned for MG's amplitude (~1 vs Lorenz ~20). Two things the section explicitly demonstrates: - the wrapper's data path handles DDE-generated targets out of the box (the data shape is all that matters downstream), - `SciMLProblemReservoir`'s untyped `prob` field would equally accept a DDEProblem as the reservoir itself; a tuned delay-coupled reservoir is left for the CTESN PR. The section also calls out, in the same plain-English voice as the Lorenz eye test, that the hyperparameters are tuned by hand for illustration and not selected via cross-validation. Adds `DelayDiffEq = "6"` to docs/Project.toml (Julia 1.10 + DelayDiffEq v6.0.3 resolves cleanly in the docs env locally).
|
Sorry for my delay on this, everything looks good! up the version and we can merge this |
PR SciML#450 introduces new public types (`SciMLProblemReservoir`, `AbstractSciMLProblemReservoir`, `AbstractSampler`, `TerminalStateSampling`) and the `RCODEReservoirExt` package extension. Discrete `ESN`/`DeepESN`/etc. API is unchanged — SemVer minor bump.
|
The GPU doc CI is failing as docs/Project.toml pins ReservoirComputing = "0.12.0" but main bumped to 0.13.0. what should I do ? widen docs compat to "0.12, 0.13" (or just "0.13"). Also I bumped up to 0.13.0 due to major changes can bump it from 0.12.23 -> 0.12.24. Which one seems apt here? |
| name = "ReservoirComputing" | ||
| uuid = "7c2d2b1e-3dd4-11ea-355a-8f6a8116e294" | ||
| version = "0.12.23" | ||
| version = "0.13.0" |
There was a problem hiding this comment.
sorry I should've specified, it should be 0.12.24 as I'm keeping 0.x for breaking changes until v1.0
PR SciML#450 is additive (new `SciMLProblemReservoir` types + package extension, no breaking changes), matching the bump convention used for the comparable LIFESN (SciML#401) and SVESM (SciML#400) additions. Also keeps the docs Project.toml compat `ReservoirComputing = "0.12.0"` satisfied without a second edit.
Master picked up an out-of-band PR SciML#457 that added @docs entries for the four SciMLProblemReservoir / AbstractSampler / TerminalStateSampling types to docs/src/api/layers.md while this PR was in review. Those docstrings were also added by this PR's own 'Continuous-Time Reservoirs' section (in the location Francesco asked for, after Wrappers), so the auto-merge left two @docs blocks declaring the same four symbols and Documenter rejected the build with 'duplicate docs found'. Resolution: drop master's section (which sat between ESN and Wrappers), keep this PR's section (after Wrappers, before Cellular Automata) per Francesco's 2026-06-14 placement request. Net effect: one Continuous-Time Reservoirs section, in the agreed location, single @docs block. Other master changes that came in cleanly: toepliz topology init (PR SciML#443), GPU.yml workflow tweak, actions/checkout v7 bump.
Skeleton-only placeholder so the draft PR exists as a discussion vehicle. No include, no export, no tests — the body errors with a clear message directing the user to the (not-yet-implemented) extension constructor. Open design questions are tracked in the PR description. Stacked on PR SciML#450; will be rebased onto master once that merges.
Summary
Implements the second deliverable of the SciML 2026 Summer Fellowship (#397) — the
RCODEReservoirExtpackage extension that makesSciMLProblemReservoiractually run. Builds on the core types from #446._collectstatespipeline: interpolated input signal →remake→solve(saveat = sample_grid)→ sampler.predictoverrides forAbstractSciMLProblemReservoir.LinearInputInterpsidesteps a DataInterpolations matrix-u/ SciMLBase dual-eltype probing interaction that crashes the bareLinearInterpolationpath inside an ODE parameter pack.solvekwargs (saveat,save_everystep,dense).:inputparameter-name collision, on degeneratetspan, and on non-NamedTupleprob.p.states[:, k]is the reservoir state after processing input columnk— inputs are placed at window starts, samples are taken at window ends. With that alignment the Euler-equivalence check holds without an off-by-one shift.Design references
args/kwargs/tspancapture (per @MartinuzziFrancesco's sketch).predict->_predictdispatch mirrors thecollectstatespattern landed in feat: SciMLProblemReservoir core types + collectstates dispatch helper #446.Test plan
Local Core suite (Julia 1.12.5): 1413/1413 pass, including 34 new extension tests covering
dx/dt = -x + u, x(0) = 0).predict(shape + determinism).T_stepsandstepsboundary errors.tspan[2] > tspan[1]enforced.:inputparameter-name collision errors.prob.paccepts NamedTuple /nothing/NullParameters; rejects other types.Roadmap for PRs 3–6 unchanged: CTESN model + Mackey-Glass/Lorenz validation (PR3), perf (PR4), LSM (PR5), DDE stretch (PR6).