Use a deterministic initial guess in solve_ode_bvp - #329
Conversation
solve_ode_bvp seeded its default initial_guess_y with np.random.rand, so every call without an explicit guess depended on global RNG state. The Poisson solvers (solve_poisson_bvp / solve_poisson_ivp) never pass a guess, making their results non-reproducible run to run and their tolerance-based tests flaky. The ODE solved here is linear, so the initial guess cannot change the converged solution, only mesh adaptation. Replace the random guess with an all-zeros guess and add a regression test that the default result is independent of np.random state.
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of concrete correctness/clarity issues in the modified docstring and the new test’s global RNG side effects that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes solve_ode_bvp reproducible by removing its dependence on NumPy’s global RNG state when initial_guess_y is not provided, which stabilizes Poisson solves that call into this routine without an explicit guess.
Changes:
- Replace the default random
initial_guess_ywith a deterministic all-zeros array insolve_ode_bvp. - Update the
solve_ode_bvpdocstring to reflect the new deterministic default. - Add a regression test asserting bit-identical results across different global RNG states.
File summaries
| File | Description |
|---|---|
src/grid/ode.py |
Makes the default BVP initial guess deterministic (zeros) and updates the docstring accordingly. |
src/grid/tests/test_ode.py |
Adds a regression test ensuring default behavior is independent of global RNG state. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| np.random.seed(0) | ||
| first = solve_ode_bvp(x, fx, coeffs, bd_cond)(probe)[0] | ||
| np.random.seed(12345) | ||
| _ = np.random.rand(50) # perturb global RNG state | ||
| second = solve_ode_bvp(x, fx, coeffs, bd_cond)(probe)[0] | ||
|
|
||
| assert_allclose(first, second, atol=0.0, rtol=0.0) | ||
| assert_allclose(first, probe**2.0 / 2.0 - probe, atol=1e-6) |
| initial_guess_y : ndarray(K, N), optional | ||
| Initial guess for :math:`y(x), \cdots, \frac{d^{K} y}{d x^{K}}` at the points `x`. |
|
@Specter842 thanks for your contribution. As it is, the core of this PR is simply to change the default initial guess from |
|
I understand thank you for your time. |
What
solve_ode_bvpseeded its defaultinitial_guess_ywithnp.random.rand(order, x.size), so any call that doesn't pass an explicit guess depends on globalnp.randomstate.solve_poisson_bvp/solve_poisson_ivpnever pass a guess, so every Poisson solve is non-reproducible run to run — it can converge or hitmax_nodesdepending on RNG state, and theatol=1e-2Poisson tests are flaky by construction (no seeding in the test suite).Change
test_solve_ode_bvp_default_initial_guess_is_deterministic: the same linear BVP solved under two different global RNG states now gives bit-identical results.Testing
pytest src/grid/tests/test_ode.py src/grid/tests/test_poisson.pypasses locally. Callers can still passinitial_guess_yexplicitly to override.