fix: reject update payloads the padding would silently misread - #607
Merged
Conversation
`Model.update` zero-pads short input up to the compiled size, which is what
lets one compiled problem serve a smaller universe. Two ways that went wrong:
A payload whose models disagreed about the universe size solved without
complaint and answered with nonsense. Giving the risk model two assets and the
bounds four leaves the padded tail carrying no risk while the bounds leave it
free, which the solver reads as riskless assets and fills:
MinVar(assets=4), 2-asset chol + 4-asset bounds
-> value 2.4e-09, weights [6e-09, 6e-09, 0.5, 0.5]
No single model can see this; each one's own inputs are consistent. `Model`
now declares `dimensions`, the (variable, size) claims its inputs imply, and
`Problem.update` collects them across all models and rejects a disagreement
with `CvxDataError`. Both bounds are declared, so a model contradicting itself
is caught by the same pass. Validation now runs over every model before the
first value is written, so a rejected payload leaves the problem untouched
rather than half-overwritten.
`dimensions` is abstract rather than a default a model may quietly not
override: `keywords` has that shape and it has already been got wrong once.
Second, input larger than the compiled problem escaped as numpy's broadcast
`ValueError`, outside the `CvxError` tree the README promises. `fill_vector`
and `fill_matrix` now raise `CvxDataError`; padding stays one-directional, as
truncating would drop assets the caller asked about. `Bounds.update` claimed
to trim in its docstring, which the helper never did.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Model.updatezero-pads short input up to the compiled size — that is what lets one compiled problem serve a universe smaller than the one it was built for. Two things went wrong at the edges of it.1. Models disagreeing about the universe solved, and solved wrongly. Give the risk model two assets and the bounds four, and the padded tail carries no risk while the bounds leave it free. The solver reads that as riskless assets and puts the portfolio there:
No exception, no bad status. No single model can see it either — each one's own inputs are internally consistent.
2. Oversized input escaped the
CvxErrortree. Handingupdatemore assets than were compiled gave numpy'sValueError: could not broadcast input array from shape (5,5) into shape (4,4), which names neither the caller's mistake nor a class the README's error table promises.What changed
Model.dimensions— new abstract method returning the(variable name, size)claims a payload implies, keyed byDataNames.WEIGHTS/FACTOR_WEIGHTS. Implemented by all seven concrete models.Problem.updatedelegates to a new_validatethat runs keyword presence and dimension agreement over every model before the first value is written. A conflict raisesCvxDataError:Inconsistent size for weights: model risk was given 2, model bound_assets was given 4fill_vector/fill_matrixraiseCvxDataErrorfor input that does not fit. Padding stays one-directional — truncating would drop assets the caller asked about.Bounds.update's docstring claimed to trim, which the helper never did.Two design notes:
keywordsuses the defaulted-with-override-obligation shape, and that contract has already been got wrong once here (themu_uncertaintybug its docstring describes). Abstract makes the omission impossible rather than documented. This is breaking for any externalModelsubclass; nothing in the repo orexperiments/subclasses it.Boundsdeclares both bounds — otherwise a short lower bound against a full-length upper bound reproduces the same riskless-tail bug from inside one model. One merge pass catches intra- and inter-model disagreement.The existing per-model shape checks are untouched: they are what a model gives you when used directly, and their tests call
model.updaterather than going throughProblem.Tests
test_aux.py— exact-fit and too-large cases for both helpers, parametrized over rows/cols/both. Also added the missingassertto the two existing tests, which callednp.allclose(...)and discarded the result.test_problem.py— the phantom-asset payload is rejected and the message names both models;Boundscontradicting itself is rejected; factor counts are checked as their own dimension; a legitimate padded universe still solves to the exact-size answer with a zero tail; a rejected payload leaves the problem solving to its previous value; an oversized payload surfaces asCvxDataErrorthroughupdate.test_holding_costs.py/test_trading_costs.py— directdimensionstests; those two models are never assembled into aBuilder.make allis green: 104 tests, 100% test and docstring coverage,ty+mypy --strictclean, no template-owned file modified.🤖 Generated with Claude Code