Correct and update how numbers are parsed from a file - #21
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new “Fraction-only” invariant for payoffmatrix is not consistently enforced across mutation paths (e.g., addrow/addcolumn) and currently conflicts with existing unit tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors numeric parsing so file inputs are converted to Fraction values more reliably and deterministically, with configurable decimal rounding, and tightens invariants so payoffmatrix is constructed from Fraction values only.
Changes:
- Replaces float-based rounding in
utils.tofraction()withDecimal-based parsing +ROUND_HALF_UP, and threadsdecimalsthrough parsing helpers. - Adds
--decimalsCLI support (and defaults) for LCP and bimatrix file parsing. - Updates/extends unit tests to cover previously failing rounding/precision cases and adjusts bimatrix initialization expectations.
File summaries
| File | Description |
|---|---|
| tests/test_lcp_units.py | Adds regression tests for decimal rounding edge cases when parsing LCP files. |
| tests/test_bimatrix_units.py | Updates payoff matrix fixture to use Fraction values and removes reliance on global decimals. |
| src/lemke/utils.py | Introduces DEFAULT_DECIMALS, validate_decimals(), and Decimal-based tofraction(); threads decimals through parsing helpers. |
| src/lemke/lemke.py | Updates LCP parsing to accept decimals and adds a --decimals CLI option. |
| src/lemke/bimatrix.py | Enforces Fraction-only payoffmatrix construction; threads decimals through bimatrix file parsing and CLI plumbing. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| AA = np.array(A, dtype=object) | ||
| if not all(isinstance(x, fractions.Fraction) for x in AA.flat): | ||
| raise TypeError("matrix must contain only Fraction values") | ||
| m, n = AA.shape |
| if not isinstance(s, str): | ||
| raise TypeError( | ||
| f"to_fraction expects a string, got {type(s).__name__}: {s!r}" | ||
| ) |
|
@nataliemes At first glance, these copilot comments looks sensible, please check them out and either fix or respond. Thanks. |
Changes
1.
decimalsis now a function argument (instead of a global variable)2. Added
--decimalsCLI option tolemke.py3. Fixed rounding/precision bugs
Since floats in python do not represent decimal numbers exactly,
tofraction()converting a string input to a float and then rounding it didn't always produce the expected result:decimalsTo fix this,
tofraction()now accepts only strings, and usesDecimalto parse and round the input half away from zero (withROUND_HALF_UP).Also, a new test was added which checks if the numbers above are now parsed and rounded correctly.
4.
payoffmatrixobject can only be created withFractionsOther arbitrary numerics (which were silently converted via
tofraction()) are no longer accepted. So,tofraction()is only used when parsing files, in other cases (like inpayoffmatrixinitialization), inputs should already beFractions.