Repository navigation
fix: reject a cluster tree that does not match the covariance columns (#814) - #817
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe shared allocation path now checks that cluster leaf values match covariance column indices. It raises ChangesAllocation validation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change makes mismatched cluster trees fail with a clear error instead of silently omitting assets or raising a bare IndexError. Valid trees are unaffected, so the merge risk is minimal. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Fixes #814.
Acceptance criterion (verbatim):
What changed
src/pyhrp/algos.py,_allocate_with: before weighting, check that the sorted leaf values equalrange(len(cov.columns)). If they don't, raiseValueErrorwith the expected and actual leaf counts and the leaf values. Bothrisk_parityandschur_risk_paritygo through this function, so bothhrpandschur_hrpare covered.root.leaves, which validates each node's children first. Malformed trees still raise their existingTypeError/ValueError, and the tests for those pass unchanged.tests/pyhrp/test_algos.py: two new tests, each run against both allocators, one per direction: a tree smaller than the universe (used to drop assets silently) and a tree larger than it (used to raise a bareIndexError).Gates
make fmt: pass (ruff, mypy, interrogate, bandit, …)make test: 136 passed, 1 skipped; coverage 100% (lines and branches)make rhiza-test: 29 passed, 6 skipped (dynamic version); the docstring examples passNot addressed
one_over_nhas the same leaf-to-index mapping but doesn't go through_allocate_with, and the issue doesn't cover it.Closes #814
🤖 Generated with Claude Code
Summary by CodeRabbit
ValueErrorduring risk-parity allocation, instead of potentially failing later with anIndexError.