fix(billing): cost-model accuracy follow-ups from the #150 review - #151
Conversation
Wrap the new _quantile_dense_cost(...) call sites at the 88-col line limit, sort tests/test_quantile_weighted_pins.py's import block, and bind the per-op loop variables (op, qval) as lambda defaults in test_weighted_ge_unweighted_and_all_four_ops to satisfy flake8-bugbear B023, matching the existing convention in tests/test_batch_underbill_pins.py::test_percentile_family_scales_with_q_count.
rfftn/rfft2/irfftn/irfft2 (2+ transform axes) and any c2c fftn/fft2/ifftn/ ifft2 call with a resized `s` now bill from numpy's real per-axis cascade instead of the final transform shape, so the previously flat 5*N*sum(log2)/ 5*(N//2)*sum(log2) pins no longer match. Updated the affected assertions in tests/test_fft_transforms_extended.py and the OP_EXPECTATIONS conformance table in tests/test_cost_convention.py to the new staged values, each with the per-stage arithmetic in a comment.
Document why pointwise out= bills the widest participating buffer (max of compute width and store width) rather than just the compute width: a wider out= store is a real materialization, and pricing it at the wider rate holds out=-casting at exact astype parity in both directions -- a wider out= costs what the equivalent astype costs, and a narrower out= never discounts the loop that actually runs. Reductions fold out= into their accumulator for a separate, genuine reason (it changes the loop numpy runs) and are unaffected. Adds tests/test_out_billing_doctrine.py to pin both directions of this behavior. No billing logic changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dba9dcc15c
ℹ️ 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".
| total += stage(ax, n, True) | ||
| current[ax] = n // 2 + 1 | ||
| remaining = range(len(axes) - 1) | ||
| remaining = reversed(remaining) if _NUMPY_GE_2_1 else remaining |
There was a problem hiding this comment.
For NumPy 2.1/2.2, which are still allowed by numpy>=2.0.0,<2.5.0, np.fft.rfftn still applies the remaining complex FFT axes in forward order; the reversal appears in 2.3+. With this cutoff, any 3+ axis rfftn case that resizes an earlier remaining axis (for example s=(4,16,4), axes=(0,1,2)) charges the reversed cascade instead of the installed NumPy's cascade, so FLOP counts are wrong for supported environments. Please gate the reverse order at the version where NumPy actually changed it.
Useful? React with 👍 / 👎.
1 participant
Summary
Four cost-model accuracy corrections plus a documentation clarification, following the forum review by @AndreiBulzan on #150. Each was reproduced against production weights and fixed with regression pins; changes are server-side cost formulas only (no weight/dtype-rate table edits, no client-surface changes —
sync_client --checkstays green).What changed
Weighted / dense-
qquantiles priced per the work they do.quantile/percentile/nanquantile/nanpercentileused a flataxis_dim + 4·q.sizeper output. Theweights=branch sorts internally, and a denseqwithmethod="inverted_cdf"returns the sorted input — both were priced linearly. Now piecewise: unweighted bills the cheaper ofkpartition passes or one shared sort-parity pass; weighted is priced at sort parity plus a per-qlookup. The scalar path is unchanged (byte-identical), so common usage is unaffected.N-D FFTs billed per execution stage.
fftn/ifftn/fft2/ifft2/rfftn/rfft2/irfftn/irfft2were priced from the final transform shape, which collapses whenspads-then-truncates. They now replay numpy's actual 1-D cascade over the evolving intermediate shapes, so the fused bill equals the sum of the equivalent explicit 1-D transforms. Therfftnremaining-axis order is version-gated (numpy changed it at 2.1.0) to stay correct across the declared 2.0–2.4 range.asarraycharges real copies.copy=True/order=materializations were billed 0. Now chargesnumel × heavier(src,dst)whenever a fresh buffer is produced (verified viamay_share_memory), 0 for a genuine view — matchingastype/copy, and aligningasarray([list])witharray([list]).selectincludes itsdefaultin dtype resolution. A widerdefault(e.g. float64 against float32 choices) promotes the output but was billed at the choice rate; it now bills at the honest output-dtype rate. Weak Python-scalar defaults stay weak per NEP 50.out=billing rationale documented (no behavior change). Clarifies in_dtype_billing.pyand the cost-model page why pointwiseout=bills the widest participating buffer (max(compute, store)width), which holdsout=-casting at exactastypeparity in both directions. Locked by parity pins.Docs regenerated (
ops.json) and the cost-model reference page refreshed for the quantile and FFT rows.Verification
pyright src/flopscope testsclean,ruff/gitlintclean.test_pocketfft.qclears sort parity, monotonic ink.Follow-ups (not in this PR)
Thanks to @AndreiBulzan for the careful report.