Fix rev7 EIRP interpolation: correct unmkpp coefficient reshape#16
Fix rev7 EIRP interpolation: correct unmkpp coefficient reshape#16nicklasorte wants to merge 1 commit into
Conversation
Rev6 had a reshape axis-swap bug: unmkpp stores coefs as [dim*pieces x order] with dim varying fastest, so the correct reshape is [dim, pieces, order]. Rev6 used [pieces, dim, order], scrambling BS/piece coefficient pairing and producing the observed semantic inversion (high query -> low EIRP, vice versa). Rev7 fixes only the reshape and downstream indexing, preserving the ~35x speedup from vectorized spline evaluation. Includes a helper-level validation script that checks absolute/relative error, correlation, and endpoints against rev5 before end-to-end integration. https://claude.ai/code/session_0189a75rK83NM29Ryy84NZ7X
![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: e47162ee31
ℹ️ 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".
| % --- Inversion detection (the rev6 failure signature) --- | ||
| % If sign(out7 - mean(out7)) is anti-correlated with sign(out5 - mean(out5)), | ||
| % we have the same axis-swap bug. | ||
| corr_val=corr(out5(:),out7(:)); |
There was a problem hiding this comment.
corr(out5(:), out7(:)) returns NaN when either vector has zero variance (for example, both helpers return all zeros in the num_cols<=1 path, or there is only one sample), and then pass_corr=corr_val>0.999 fails even when rev5 and rev7 are numerically identical. This causes false validation failures and can block integration on valid inputs; add a zero-variance/short-vector guard before applying the correlation threshold.
Useful? React with 👍 / 👎.
2 participants
Summary
Corrects the fast-path Monte Carlo EIRP interpolation (rev7) by fixing the reshape dimension order when extracting piecewise polynomial coefficients from
unmkpp. This ensures rev7 produces numerically identical results to the golden rev5 implementation.Changes
monte_carlo_super_bs_eirp_dist_rev7.m: Fixed reshape of
unmkppcoefficient matrix from[pieces, dim, order]to correct[dim, pieces, order]layout. Theunmkppfunction returns coefficients as[dim*pieces x order]with dimension indices varying fastest within each piece, requiring the corrected reshape to properly index coefficients by BS row and piece.validate_monte_carlo_super_bs_eirp_dist_rev5_rev7.m: Added comprehensive helper-level validation function that:
Implementation Details
https://claude.ai/code/session_0189a75rK83NM29Ryy84NZ7X