fix: improve Poisson solver IVP robustness (#162)#303
Open
alok-108 wants to merge 2 commits into
Open
Conversation
- Adaptive r_interval based on density cumulative integral - Fallback ODE solver methods chain - Added test suite for IVP robustness Closes theochem#162
Contributor
There was a problem hiding this comment.
Pull request overview
Addresses numerical instability in the Poisson IVP solver (#162) by adding adaptive integration bounds, a multi-method ODE fallback chain, noise filtering for higher-l components, and safe analytic extrapolation outside the radial domain.
Changes:
poisson.py: Adaptiver_maxfrom cumulative |density|, skip ODE integration for negligible l>0 components, wrap returned splines withmake_safe_splinefor r > r_max analytic continuation.ode.py: Replace single-methodsolve_ivpcall with a fallback chain (DOP853 → RK45 → BDF → Radau → LSODA), warning on each failure and raising only if all fail.tests/test_poisson_ivp_robustness.py: New tests for IVP-vs-BVP agreement, fallback warning behavior, and adaptiver_maxheuristic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/grid/poisson.py | Adaptive r_max, l>0 zero-source skip, safe-spline boundary wrapper |
| src/grid/ode.py | Multi-method ODE solver fallback chain with warnings |
| src/grid/tests/test_poisson_ivp_robustness.py | New robustness tests for IVP solver |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Author
|
Hi @marco-2023, here's a visual summary of the changes in this PR. All Copilot review comments have been addressed. Please take a look when you can. Thanks! 🔍 What this PR adds (flow)graph TD
A[Poisson IVP call] --> B[Adaptive r_max<br/>from density decay]
B --> C[For each l,m component]
C --> D{Source term<br/>negligible?}
D -- yes --> E[Skip ODE,<br/>return zero spline]
D -- no --> F[Try DOP853]
F -- success --> G[Wrap in safe spline]
F -- fail --> H[Fallback chain:<br/>RK45 → BDF → Radau → LSODA]
H -- success --> G
H -- all fail --> I[Raise RuntimeError]
G --> J[Interpolate safely<br/>even outside domain]
J --> K[Return potential]
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses the numerical instabilities in the Poisson Solver Initial Value Problem (IVP) implementation as described in Issue #162.
When solving Poisson's equation as an IVP, inward integration for higher angular momentum modes ($l > 0$ ) is notoriously susceptible to catastrophic amplification of numerical noise from the starting boundary. To make
solve_poisson_ivprobust across both compact and diffuse charge densities, this PR introduces several mathematical and programmatic safeguards:Key Changes:
Adaptive Integration Bounds (
r_max):Instead of using a fixed or excessively large starting radius, the integration bound is now adaptively scaled based on the density's decay. By calculating the cumulative integral of the radial density, we estimate
r_99(the radius enclosing 99% of the charge) and capr_maxto prevent integrating over unnecessarily massive distances (e.g., fromr=1000) for highly compact densities.Fallback ODE Solver Chain:
The highly-accurate
DOP853is maintained as the default solver. However, if it fails to converge due to stiffness or step-size limits,solve_ode_ivpnow implements a graceful fallback chain (DOP853→RK45→BDF→Radau→LSODA) and raises a warning instead of immediately crashing with aValueError.Filtering Numerical Noise ($l > 0$ ):$l > 0$ radial components. The solver previously amplified this tiny noise to $10^{24}$ ! We now threshold components where the maximum absolute value is
For spherically symmetric charge densities, numerical integration on the Lebedev grid leaves tiny precision errors (e.g.,
~1e-16) in the< 1e-12and skip the ODE integration entirely, returning a zero spline to avoid artificial blowups.Safe Boundary Extrapolation ($l=0$ , and $l>0$ ) beyond the boundary, rather than letting the
make_safe_spline):Added a safeguard to handle evaluations outside the transform domain, returning the correct analytical behavior (
Q/rfor the monopole0forCubicSplineextrapolate blindly.Robustness Test Suite:
Added
tests/test_poisson_ivp_robustness.pyto explicitly verify that the method is now stable for both compact and diffuse densities, and to ensure the fallback warning mechanism works correctly.Closes #162