Conversation
Introduce a shared interpolation layer in core/quantiles.py and refactor fixed-knot models (TiRex, TimesFM, TabPFN, FlowState, Tafsut, Toto 2.0) to always run native knots then interpolate to any requested level or quantile with edge clamping. Remove the undocumented level=0 sentinel from QuantileConverter and allow PatchTST-FM to pass arbitrary quantiles natively. Co-authored-by: Cursor <cursoragent@cursor.com>
Merging this PR will improve performance by 13.07%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_model_forecast[moirai-1.1] |
1.6 s | 1.4 s | +13.07% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing feat/quantile-interpolation (a0c243d) with main (d6cc53f)
Always pass DEFAULT_QUANTILES_TFM when loading TimesFM v1, and apply resolve_quantile_values for v2/v3 so arbitrary levels work without the legacy exact-length guard. Co-authored-by: Cursor <cursoragent@cursor.com>
Re-run tirex-family with level=[20, 40, 60, 80] and refresh timesfm-family markdown and code cells to match the new quantile interpolation behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove the allclose guard that raised on 8 vs 9 quantiles and apply resolve_quantile_values like other fixed-knot models. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codspeedbot explain this regression |
Explanation:
|
| Component | BASE | HEAD | Change |
|---|---|---|---|
| Instructions | 7.77 µs | 9.19 µs | +18.2% |
| Cache misses | 14.25 µs | 17.08 µs | +19.9% |
| Memory accesses | 75.53 µs | 83.69 µs | +10.8% |
| CPU total | 97.6 µs | 110 µs | +12.7% |
The instruction count is hardware-independent for a pure-Python benchmark like this one, so the +18% there is genuinely caused by the code change — the flagged runner difference (AMD EPYC → Intel Xeon) can move the cache/memory estimates, but it cannot invent extra executed instructions.
Where the extra work comes from
The benchmark measures exactly this:
qc = QuantileConverter(level=[80, 95])
return qc.quantilesand the PR changed that path from a direct loop over level to:
validated_level = validate_levels(level) # extra call frame + list(level) + any(genexpr)
assert validated_level is not None
for lv in validated_level:
...I reproduced this locally with Callgrind on an isolated reproduction of both versions (20k iterations, startup subtracted):
| Variant | Instructions / call |
|---|---|
main (direct loop) |
18,771 |
this PR (validate_levels) |
23,597 (+25.7%) |
The ~4.8k extra instructions are the cost of an extra Python frame, the list(level) copy, and the generator created by any(lv == 0 for lv in levels) — all for a 2-element list. The extra allocations also explain the bump in the memory-access component, which is the dominant term here (~77% of the total).
Does it matter?
Practically, no. QuantileConverter is constructed once per forecast() call, so ~12 µs sits next to model inference measured in seconds. This benchmark is a pure micro-benchmark of the conversion path, which is why a few thousand instructions show up as a double-digit percentage.
If you want the µs back
Folding the zero check into the existing loop keeps the same validation and recovers most of the cost (measured the same way):
if quantiles is None and level is not None:
validated_level = list(level)
_quantiles = []
for lv in validated_level:
if lv == 0:
raise ValueError(_LEVEL_ZERO_ERROR)
q_lo, q_hi = QuantileConverter._level_to_quantiles(lv)
...→ 19,805 instructions/call, i.e. ~+5% over main instead of +26%. The trade-off is losing validate_levels() as a shared, separately tested helper (it currently has only this one call site plus its unit tests).
I did not change any code — say the word and I'll apply that micro-optimization, or feel free to acknowledge the regression on CodSpeed if you'd rather keep the cleaner helper.
Note: the ⚡ +18.77% on test_model_forecast[moirai-1.1] is a WallTime benchmark that also ran across the runner difference, so I would not read it as a real speedup from this PR.
Summary
- Warming up the performance wizard...
- Investigate the regression — Compared base (d6cc53f) and head (408fa7c) metric breakdowns: +18% instructions, +11% memory accesses
- Measure instruction delta locally — Callgrind: new validate_levels path adds ~4.8k instructions per QuantileConverter construction (+26%)
- Handling comment — Comment handling finished successfully
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.
There was a problem hiding this comment.
🟡 Changes recommended
Several quantile paths can return incorrect or duplicate output columns, and PatchTST-FM may use a requested quantile instead of the point forecast.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 4
- Review effort level: Lite
- Inline level=0 check to avoid CodSpeed regression in QuantileConverter - Fix single-knot interpolate_quantiles axis restoration - Reject duplicate quantile column names from int(q*100) collisions - Strip native TimesFM v1 q-columns before assigning resolved output - Always request q=0.5 in PatchTST-FM backend for correct point forecasts Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🟡 Changes recommended
Interpolation currently misaligns multi-row outputs, arbitrary quantile column collisions can overwrite results, and the TabPFN example still uses the removed sentinel.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
foundationforecast/models/toto.py:340
- Toto 2.0 now resolves arbitrary quantiles, but
forecast()still writes the returned values with the oldint(q * 100)loop at lines 414–417. A valid request such as[0.151, 0.159]maps both values to*-q-15, so one forecast overwrites the other instead of returning both; use_assign_quantile_forecastshere as the other interpolating models do.
- Files reviewed: 17/17 changed files
- Comments generated: 3
- Review effort level: Lite
| kind="linear", | ||
| assume_sorted=True, | ||
| ) | ||
| out = interp(requested).T.reshape(*orig_shape, len(requested_quantiles)) |
| if quantiles is None: | ||
| quantile_levels = DEFAULT_QUANTILES | ||
| else: | ||
| quantile_levels = sorted(set(quantiles) | {0.5}) |
| if 0 in level: | ||
| raise ValueError(_LEVEL_ZERO_ERROR) |
Summary
foundationforecast/core/quantiles.pywith vectorized linear interpolation and edge clamping for fixed-knot modelslevelorquantileslevel=0sentinel fromQuantileConverter(now raises a clear error)docs/forecasting-parameters.mdand example notebooksAPI after this change
Edge clamping
When a requested quantile falls outside the model's native knot range, the forecast at the nearest edge knot is returned (same semantics as
numpy.interp). For example, on knots0.1–0.9,quantiles=[0.01]returns the same values asquantiles=[0.1].Test plan
tests/core/test_quantiles.py— interpolation, subset selection, edge clampingtests/core/test_forecaster.py—level=0rejection, level/quantile conversiontests/models/test_models.py— updated to uselevel=[20, 40, 60, 80]without sentinelMade with Cursor