refactor: de-duplicate DCC math and cut build_exp5 complexity - #474
Conversation
Addresses the three below-10 scorecard findings from the rhiza v1.1.3 boost. - #472: optimize.py's _dcc_correlation reimplemented the Engle-DCC covariance/ correlation math that already lives in Experiment5.py (dcc_correlation -> ewm_covariance + correlation_from_covariance). Pull dcc_correlation from the loaded notebook namespace instead, so the notebook is the single source of truth. Deletes the duplicate. - #471: removing _dcc_correlation drops one B(6) block; build_exp5's per-day solve loop is extracted into _solve_positions + _day_position. No block in optimize.py now ranks above CC grade A (was two B(6) blocks). - #473: our own portfolio assembly used the deprecated polars concat(how="horizontal") (Experiment4/5.py), whose default is changing in a future release and could perturb the tightly-pinned Sharpe baselines. Switch to the explicit, stable how="horizontal_extend" (matching optimize.py) and document the drift-resistance rationale in SHARPE_PINS.md. Heights are equal so the result is numerically identical. All gates pass; 76 tests green at 100% coverage. The pinned Sharpe baselines (notebook end-to-end and build_exp* builders, all 5 experiments) are unchanged, confirming the refactor and the concat switch are behaviour-preserving. Closes #471 Closes #472 Closes #473 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Optuna-based optimizer for the marimo CTA notebooks to remove duplicated Engle-DCC math and reduce cyclomatic complexity, while also hardening the pinned-Sharpe portfolio builders against Polars API default drift.
Changes:
- Removed the duplicated DCC correlation implementation from
optimize.pyand re-usedExperiment5.py’sdcc_correlationvia the executed notebook namespace. - Reduced
build_exp5complexity by extracting per-day position solving into_solve_positionsand_day_position, preserving the “skip degenerate days” behavior. - Switched notebook portfolio assembly to
pl.concat(..., how="horizontal_extend")(and documented the rationale) to avoid future default changes affecting pinned Sharpe baselines.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/development/SHARPE_PINS.md | Documents why builders should prefer explicit, stable dependency APIs (Polars concat mode) to protect pinned-Sharpe baselines. |
| book/marimo/notebooks/optimize.py | De-duplicates DCC math by reusing Experiment 5 helpers and reduces build_exp5 complexity via extracted helpers. |
| book/marimo/notebooks/Experiment5.py | Updates portfolio assembly concat mode to horizontal_extend for stability. |
| book/marimo/notebooks/Experiment4.py | Updates portfolio assembly concat mode to horizontal_extend for stability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Addresses the three below-10 scorecard findings raised during the rhiza v1.1.3 boost (#470): #471 (code complexity), #472 (architecture / duplication), #473 (baseline test hardening). All in locally-owned
book/marimo/notebooks/.Changes
#472 — de-duplicate the DCC correlation helper (architecture 9→10)
optimize.py's_dcc_correlationreimplemented the exact Engle-DCC covariance/correlation math already defined inExperiment5.py(dcc_correlation→ewm_covariance+correlation_from_covariance). Sinceoptimize.pyalready executes the notebooks viarunpyand those helpers are module-level@app.functions,build_exp5now pullsdcc_correlationstraight from the loaded notebook namespace. The duplicated implementation is deleted — the notebook is the single source of truth.#471 — cut
optimize.pycomplexity to grade A (complexity 9→10)_dcc_correlationeliminates oneB(6)block.build_exp5's per-day matrix-solve loop is extracted into_solve_positions+_day_position.build_exp5is nowA(1); the helpers areA(4).radon cc book/marimo/notebooks/optimize.pyreports no block above grade A (was twoB(6)blocks).inv_a_norm/solveare still resolved from module globals, sotest_build_exp5_skips_singular_days's monkeypatch keeps working.#473 — harden the pinned-Sharpe builders against dependency drift (test design 9→10)
Our own portfolio assembly used the deprecated polars
concat(how="horizontal")(Experiment4.py,Experiment5.py), whose default behaviour is changing in a future release — a silent flip that could perturb the 1e-6-pinned Sharpe baselines. Switched both to the explicit, stablehow="horizontal_extend"(matchingoptimize.py) and documented the drift-resistance rationale inSHARPE_PINS.md. Row heights are equal, so the result is numerically identical. (The tolerance itself is already centralized inexpected_sharpe.pyand documented inSHARPE_PINS.md, which explicitly forbids widening it — so no tolerance change was made.)Verification
All gates pass:
make fmtmake typecheckmake docs-coveragemake deptrymake securitymake testThe pinned Sharpe baselines (both notebook-end-to-end and the
build_exp*builders, all 5 experiments) are unchanged, confirming the refactor and theconcatswitch are behaviour-preserving. The 120 remainingDeprecationWarnings now originate solely fromjquantstatsinternals (upstream); our code contributes none.Closes #471
Closes #472
Closes #473
🤖 Generated with Claude Code