|
| 1 | +# Issues found — FDE/PDE integrators (2026-06-19) |
| 2 | + |
| 3 | +Reviewer: senior numerical-methods + JAX expert (fractional differential equations). |
| 4 | +Scope: `brainpy/integrators/fde/{Caputo,GL,base,generic}.py`, `brainpy/integrators/pde/base.py`. |
| 5 | +Environment: brainpy 2.7.8, jax 0.10.2, scipy (CPU). |
| 6 | + |
| 7 | +Core numerics were validated head-to-head against independent reference implementations |
| 8 | +(rectangle product-integration Caputo–Euler [Li & Zeng 2013], L1 scheme, and the |
| 9 | +Grünwald–Letnikov short-memory recurrence), single- and multi-variable, with distinct |
| 10 | +per-variable `alpha`, both inside and beyond the memory window. **All three solvers match |
| 11 | +their analytic/reference recurrences to machine precision** — the previously-reported |
| 12 | +numerical bugs (initial-condition mis-scaling, coefficient order, memory truncation) are |
| 13 | +**not present** in this worktree (see "Already fixed" section). |
| 14 | + |
| 15 | +--- |
| 16 | + |
| 17 | +### P7-H1 — `CaputoEuler.reset` replaces memory `Variable`s with plain `Array`s (state desync) [High] |
| 18 | +- File: brainpy/integrators/fde/Caputo.py:173 (and 172 is fine) |
| 19 | +- Category: correctness / JAX-semantics |
| 20 | +- What: `reset()` does `self.f_states[key] = bm.zeros(...)`. `self.f_states` is a plain |
| 21 | + Python `dict` (created at `__init__`, line 160), not a `bm.VarDict`, so this assignment |
| 22 | + *replaces* the registered `bm.Variable` with a bare `bm.Array`. The `bm.Variable` |
| 23 | + originally registered via `register_implicit_vars(self.f_states)` is now orphaned: it is |
| 24 | + still returned by `self.vars()` (stale), while `_integral_func` writes the memory buffer |
| 25 | + into the *new* `Array` object. |
| 26 | +- Why it's a bug: any JAX-transformed re-run after `reset` (the documented way to re-run |
| 27 | + from new initial values, e.g. via `IntegratorRunner`) snapshots/restores the **stale** |
| 28 | + `Variable` from `self.vars()` while the integral function reads/writes the **new** `Array`, |
| 29 | + so the convolution memory is desynced and results are silently wrong. (`CaputoL1Schema` |
| 30 | + and `GLShortMemory` are immune — they store their memory in `bm.VarDict`, whose |
| 31 | + `__setitem__` does in-place `.value` assignment.) |
| 32 | +- Repro: |
| 33 | + ```python |
| 34 | + import brainpy as bp, brainpy.math as bm, numpy as np |
| 35 | + bm.enable_x64() |
| 36 | + intg = bp.fde.CaputoEuler(lambda y, t: -y, alpha=0.8, num_memory=100, inits=[1.]) |
| 37 | + intg.reset([1.]) |
| 38 | + assert isinstance(intg.f_states['y'], bm.Variable) # FAILS: it is a bm.Array |
| 39 | + runner = bp.IntegratorRunner(intg, monitors=['y'], dt=0.05, inits=[1.]) |
| 40 | + runner.run(1.0) |
| 41 | + # last y == 0.911 (wrong); a fresh integrator (no reset) gives 0.380 (correct) |
| 42 | + ``` |
| 43 | +- Fix: store `f_states` in a `bm.VarDict` and reset via in-place `.value` assignment so the |
| 44 | + registered `Variable` identity is preserved. |
| 45 | +- Tests: `Caputo_test.py::TestCaputoEulerReset::test_reset_preserves_variable`, |
| 46 | + `::test_reset_then_run_matches_fresh` |
| 47 | +- Status: fixed |
| 48 | + |
| 49 | +--- |
| 50 | + |
| 51 | +### P7-M1 — `fdeint(method=...)` default makes `set_default_fdeint` a no-op [Medium] |
| 52 | +- File: brainpy/integrators/fde/generic.py:35 (default), used at :63 |
| 53 | +- Category: api-drift / correctness |
| 54 | +- What: `fdeint(..., method='l1', ...)` has the *literal* default `'l1'`. The body then does |
| 55 | + `method = _DEFAULT_FDE_METHOD if method is None else method`, but `method` is never `None` |
| 56 | + when the caller omits it, so the `_DEFAULT_FDE_METHOD` global (settable via |
| 57 | + `set_default_fdeint`) is ignored for default-method calls. |
| 58 | +- Why it's a bug: `set_default_fdeint('euler')` followed by `fdeint(...)` still builds a |
| 59 | + `CaputoL1Schema`, contradicting the documented purpose of `set_default_fdeint` / |
| 60 | + `get_default_fdeint`. The public default-method mechanism is dead for the common path. |
| 61 | +- Repro: |
| 62 | + ```python |
| 63 | + from brainpy.integrators.fde.generic import fdeint, set_default_fdeint |
| 64 | + set_default_fdeint('euler') |
| 65 | + type(fdeint(alpha=0.8, num_memory=20, inits=[1.], f=lambda y, t: -y)).__name__ |
| 66 | + # 'CaputoL1Schema' (expected 'CaputoEuler') |
| 67 | + ``` |
| 68 | +- Fix: change the keyword default to `method=None` so the `_DEFAULT_FDE_METHOD` fallback |
| 69 | + actually runs. |
| 70 | +- Tests: `generic`-level test in `Caputo_test.py::TestFdeintDefaultMethod::test_set_default_fdeint_respected` |
| 71 | +- Status: fixed |
| 72 | + |
| 73 | +--- |
| 74 | + |
| 75 | +## Already fixed in this worktree (verified, recorded only) |
| 76 | + |
| 77 | +These were reported against an earlier revision (`dev/issues-found-20260618.md`, FDE block) |
| 78 | +and are **already corrected** in the code under review. Re-verified here; no further action. |
| 79 | + |
| 80 | +- **C-08 / `CaputoEuler` initial-condition scaling** — `Caputo.py:211` now reads |
| 81 | + `self.inits[key] + integral * (dt**alpha/alpha)` with `integral = coef @ f_states`, i.e. |
| 82 | + `y0` is added *outside* the `dt^alpha/alpha` scaling. Verified `D^a y=0, y0=1 → y≡1` and a |
| 83 | + full `f=y, y0=2` reference run match to 1e-15. Status: recorded-only (no change needed). |
| 84 | +- **H-30 / `GLShortMemory.reset` KeyError** — `GL.py:187` uses `key + '_delay'`. `reset` runs |
| 85 | + cleanly. Status: recorded-only. |
| 86 | +- **H-31 / `CaputoL1Schema.hists()` ValueError** — `Caputo.py:384` uses `.items()`. `hists()` |
| 87 | + returns a dict cleanly. Status: recorded-only. |
| 88 | +- **H-32 / `set_default_fdeint` wrong global** — `generic.py:87-88` assigns |
| 89 | + `_DEFAULT_FDE_METHOD`. `get_default_fdeint()` reflects the set value. Status: recorded-only. |
| 90 | + (But see P7-M1: the value is then ignored by `fdeint`'s literal default.) |
| 91 | + |
| 92 | +--- |
| 93 | + |
| 94 | +## Low (recorded only — not fixed per task policy) |
| 95 | + |
| 96 | +- **P7-L1** — `Caputo.py:192` type-checks `isinstance(devs, (bm.ndarray, jax.Array))` while |
| 97 | + `GL.py`/`CaputoL1Schema` use `bm.Array`. `bm.ndarray is bm.Array`, so this is cosmetic |
| 98 | + inconsistency only. Category: style. |
| 99 | +- **P7-L2** — `generic.py:36` annotates `dt: str = None` in `fdeint`; should be |
| 100 | + `dt: float = None`. Category: style/typing. |
| 101 | +- **P7-L3** — Docstrings of `CaputoL1Schema`/`GLShortMemory` say "fractional order in (0, 1)" |
| 102 | + in the `UnsupportedError` message, but both classes (correctly) accept `alpha == 1`. |
| 103 | + The class-level `Parameters` docstrings say `(0., 1.]`. Message/docstring drift only. |
| 104 | + Category: style/docs. |
| 105 | +- **P7-L4** — Pervasive `Parameters::` / `Returns::` / `References::` / `Examples::` |
| 106 | + literal-block markers (won't render as NumPy-doc sections, violates CLAUDE.md). Present in |
| 107 | + all FDE files. Category: style/docs. |
| 108 | +- **P7-L5** — `Caputo.py:50` docstring typo "may be arbitrary real numbers" written as |
| 109 | + "ay be"; `generic.py:107` "name: ste". Category: style/docs. |
| 110 | +- **P7-L6** — `pde/base.py` `PDEIntegrator(Integrator): pass` is an unused stub with no PDE |
| 111 | + solvers, no docstring, not in any `__all__`. No functional bug; documents intent only. |
| 112 | + Category: style/dead-code. |
| 113 | + |
| 114 | +--- |
| 115 | + |
| 116 | +## Out-of-scope / cross-cutting (left unfixed) |
| 117 | + |
| 118 | +None. All identified Critical/High/Medium issues are inside scope and were fixed. |
0 commit comments