feat(calc): formulas fallback + 18 new builtins (lookup & conditional)#3
feat(calc): formulas fallback + 18 new builtins (lookup & conditional)#3
Conversation
Wire the `formulas` library as a formula-level fallback for functions not covered by builtins (financial: PMT, SLN, NPV; lookup: VLOOKUP, HLOOKUP, INDEX/MATCH; etc). Multi-column ranges are reshaped to 2D numpy arrays so lookup functions receive proper table_array shapes. Add 10 new builtin implementations (25 total): - Math: ROUNDDOWN, MOD, POWER, SQRT, SIGN - Text: LEFT, RIGHT, MID, LEN, CONCATENATE Add range_shape() utility for computing (n_rows, n_cols) from range references. 28 new tests covering builtins, fallback, perturbation propagation, and combined evaluation chains. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a two-tier formula evaluation strategy to the wolfxl calc engine: 25 builtin function implementations for fast, deterministic evaluation, plus a fallback to the formulas library for 200+ Excel functions not covered by builtins. The PR adds 10 new builtin functions (ROUNDDOWN, MOD, POWER, SQRT, SIGN, LEFT, RIGHT, MID, LEN, CONCATENATE) and introduces infrastructure to compile and cache formulas library functions with proper cell reference resolution and numpy array reshaping for multi-dimensional ranges.
Changes:
- Added 10 new builtin implementations for math and text functions (total 25 builtins)
- Integrated formulas library as lazy fallback with compilation caching and parameter resolution
- Added range_shape utility to compute dimensions for multi-column range reshaping
- Added 28 comprehensive integration tests covering builtins, fallback with constants, fallback with cell references, perturbation propagation, and combined scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| tests/test_calc_formulas_integration.py | New test file with 28 tests covering all 10 new builtins and formulas library fallback scenarios |
| python/wolfxl/calc/_parser.py | Added range_shape utility to compute (rows, cols) for range references |
| python/wolfxl/calc/_functions.py | Added 10 new builtin implementations (5 math, 5 text) and registered them in _BUILTINS |
| python/wolfxl/calc/_evaluator.py | Added formulas library detection, fallback mechanism with compilation caching, parameter resolution, and result normalization |
Comments suppressed due to low confidence (1)
python/wolfxl/calc/_evaluator.py:292
- The _compiled_cache dictionary is never cleared when load() is called. If a workbook is reloaded with different formulas, the cache could contain stale compiled functions from the previous workbook. Consider clearing the cache in the load() method alongside _cell_values and _graph.
def load(self, workbook: Workbook) -> None:
"""Scan workbook, store cell values, build dependency graph."""
self._cell_values.clear()
self._graph = DependencyGraph()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(args) < 1 or len(args) > 2: | ||
| raise ValueError("LEFT requires 1 or 2 arguments") | ||
| text = _coerce_string(args[0]) | ||
| num_chars = int(_coerce_numeric([args[1]])[0]) if len(args) > 1 else 1 |
There was a problem hiding this comment.
The LEFT function doesn't handle negative num_chars values, which would cause unexpected behavior. In Excel, LEFT with a negative number of characters returns a #VALUE! error. Consider adding validation to check if num_chars is negative and raise an appropriate error.
| num_chars = int(_coerce_numeric([args[1]])[0]) if len(args) > 1 else 1 | |
| num_chars = int(_coerce_numeric([args[1]])[0]) if len(args) > 1 else 1 | |
| if num_chars < 0: | |
| raise ValueError("LEFT: num_chars must be non-negative") |
There was a problem hiding this comment.
Fixed in 73831a1. LEFT now returns #VALUE\! for negative num_chars, with test.
| nums = _coerce_numeric(args) | ||
| if len(nums) != 2: | ||
| raise ValueError("POWER: non-numeric argument") | ||
| return nums[0] ** nums[1] |
There was a problem hiding this comment.
The POWER function doesn't handle the edge case of negative base with a fractional exponent, which would raise a ValueError in Python (complex result). Excel returns #NUM! error in this case. Consider adding validation to check for this condition and return an appropriate error value.
| return nums[0] ** nums[1] | |
| base, exponent = nums[0], nums[1] | |
| # Excel returns a numeric error (#NUM!) when raising a negative base | |
| # to a non-integer exponent. Mimic this by rejecting such inputs | |
| # instead of returning a complex result. | |
| if base < 0 and not float(exponent).is_integer(): | |
| raise ValueError("POWER: invalid for negative base with non-integer exponent") | |
| return base ** exponent |
There was a problem hiding this comment.
Fixed in 73831a1. POWER now returns #NUM! for negative base with fractional exponent. Test added.
python/wolfxl/calc/_parser.py
Outdated
| n_rows = abs(end_row - start_row) + 1 | ||
| n_cols = abs(end_col - start_col) + 1 |
There was a problem hiding this comment.
The range_shape function uses abs() to calculate n_rows and n_cols, which means it would work correctly even if the range is specified in reverse order (e.g., C3:A1). However, this is inconsistent with the expand_range function which normalizes the order explicitly using min/max. While this works, consider using the same pattern (min/max) for consistency with the rest of the codebase.
| n_rows = abs(end_row - start_row) + 1 | |
| n_cols = abs(end_col - start_col) + 1 | |
| top_row = min(start_row, end_row) | |
| bottom_row = max(start_row, end_row) | |
| left_col = min(start_col, end_col) | |
| right_col = max(start_col, end_col) | |
| n_rows = bottom_row - top_row + 1 | |
| n_cols = right_col - left_col + 1 |
There was a problem hiding this comment.
Fixed in 73831a1. Switched to min/max for consistency with expand_range().
| if ':' in param: | ||
| # Range parameter - resolve to numpy array | ||
| # Qualify with sheet name for range_shape parsing | ||
| qualified = param if '!' in param else f"{sheet}!{param}" | ||
| values = self._resolve_range(param, sheet) | ||
| flat = np.array([v if v is not None else 0 for v in values]) | ||
| n_rows, n_cols = range_shape(qualified) | ||
| if n_cols > 1 and flat.size == n_rows * n_cols: | ||
| flat = flat.reshape(n_rows, n_cols) | ||
| args.append(flat) |
There was a problem hiding this comment.
When converting None values to 0 for numpy arrays (line 636), this could lead to incorrect results for functions that should distinguish between empty cells and zeros. For example, COUNTA should count non-empty cells, but this conversion would make None cells appear as zeros. Consider preserving None values or using numpy's nan for missing values, depending on what the formulas library expects.
There was a problem hiding this comment.
Intentional: the formulas library requires numpy arrays with numeric values. The None→0 coercion is scoped to the formulas fallback path only - builtin functions like COUNTA handle None directly. Using np.nan would break formulas lib function signatures that expect float arrays.
| class TestBuiltinLeft: | ||
| def test_basic(self) -> None: | ||
| wb = wolfxl.Workbook() | ||
| ws = wb.active | ||
| ws["A1"] = "Hello World" | ||
| ws["B1"] = '=LEFT(A1,5)' | ||
| ev = WorkbookEvaluator() | ||
| ev.load(wb) | ||
| results = ev.calculate() | ||
| assert results["Sheet!B1"] == "Hello" | ||
|
|
||
| def test_default_one_char(self) -> None: | ||
| wb = wolfxl.Workbook() | ||
| ws = wb.active | ||
| ws["A1"] = "Hello" | ||
| ws["B1"] = '=LEFT(A1)' | ||
| ev = WorkbookEvaluator() | ||
| ev.load(wb) | ||
| results = ev.calculate() | ||
| assert results["Sheet!B1"] == "H" | ||
|
|
||
|
|
||
| class TestBuiltinRight: | ||
| def test_basic(self) -> None: | ||
| wb = wolfxl.Workbook() | ||
| ws = wb.active | ||
| ws["A1"] = "Hello World" | ||
| ws["B1"] = '=RIGHT(A1,5)' | ||
| ev = WorkbookEvaluator() | ||
| ev.load(wb) | ||
| results = ev.calculate() | ||
| assert results["Sheet!B1"] == "World" | ||
|
|
||
|
|
||
| class TestBuiltinMid: | ||
| def test_basic(self) -> None: | ||
| wb = wolfxl.Workbook() | ||
| ws = wb.active | ||
| ws["A1"] = "Hello World" | ||
| ws["B1"] = '=MID(A1,7,5)' | ||
| ev = WorkbookEvaluator() | ||
| ev.load(wb) | ||
| results = ev.calculate() | ||
| assert results["Sheet!B1"] == "World" | ||
|
|
||
|
|
||
| class TestBuiltinLen: | ||
| def test_basic(self) -> None: | ||
| wb = wolfxl.Workbook() | ||
| ws = wb.active | ||
| ws["A1"] = "Hello" | ||
| ws["B1"] = '=LEN(A1)' | ||
| ev = WorkbookEvaluator() | ||
| ev.load(wb) | ||
| results = ev.calculate() | ||
| assert results["Sheet!B1"] == 5.0 | ||
|
|
||
|
|
||
| class TestBuiltinConcatenate: | ||
| def test_basic(self) -> None: | ||
| wb = wolfxl.Workbook() | ||
| ws = wb.active | ||
| ws["A1"] = "Hello" | ||
| ws["A2"] = " " | ||
| ws["A3"] = "World" | ||
| ws["B1"] = '=CONCATENATE(A1,A2,A3)' | ||
| ev = WorkbookEvaluator() | ||
| ev.load(wb) | ||
| results = ev.calculate() | ||
| assert results["Sheet!B1"] == "Hello World" | ||
|
|
There was a problem hiding this comment.
Missing test coverage for negative num_chars in LEFT, RIGHT, and MID functions. These edge cases should be tested to ensure the functions properly handle or reject invalid inputs, as noted in the function implementation issues.
There was a problem hiding this comment.
Fixed in 73831a1. Added edge case tests for LEFT(-1), MID(0,.), MID(.,-1), and POWER(-1, 0.5).
| import formulas as fm | ||
| import numpy as np |
There was a problem hiding this comment.
The formulas library and numpy are imported inside the _formulas_fallback method rather than at module level. While this lazy importing pattern is intentional to avoid dependencies when the formulas library is not installed, it means these imports will be executed on every call to _formulas_fallback. Consider moving the imports to the top of the method or caching the imported modules as class attributes after the first import.
| import formulas as fm | |
| import numpy as np | |
| # Lazily import and cache the optional formulas and numpy modules | |
| fm = getattr(self, "_formulas_module", None) | |
| np = getattr(self, "_numpy_module", None) | |
| if fm is None or np is None: | |
| try: | |
| import formulas as _fm # type: ignore[import-not-found] | |
| import numpy as _np | |
| except ImportError: | |
| logger.debug( | |
| "formulas fallback requested, but 'formulas' and/or 'numpy' " | |
| "are not installed" | |
| ) | |
| return None | |
| self._formulas_module = fm = _fm | |
| self._numpy_module = np = _np |
There was a problem hiding this comment.
The lazy import is intentional - formulas and numpy are optional extras (wolfxl[calc]). This method only runs as a fallback for functions without builtins. Module-level caching would add complexity for marginal benefit since Python already caches imported modules in sys.modules.
| import pytest | ||
|
|
There was a problem hiding this comment.
The pytest import on line 9 is unused. Consider removing it or adding tests that use pytest features like parametrize or raises.
| import pytest |
There was a problem hiding this comment.
Resolved by 73831a1 - pytest is now used by the _requires_formulas skip marker.
| text = _coerce_string(args[0]) | ||
| start = int(_coerce_numeric([args[1]])[0]) | ||
| num_chars = int(_coerce_numeric([args[2]])[0]) | ||
| # Excel MID is 1-indexed |
There was a problem hiding this comment.
The MID function doesn't validate that start and num_chars are positive. In Excel, MID with start < 1 returns #VALUE! error, and num_chars < 0 also returns #VALUE!. The current implementation would silently handle negative values incorrectly (e.g., start=0 would access text[-1]).
| # Excel MID is 1-indexed | |
| # Excel MID is 1-indexed; start must be >= 1 and num_chars >= 0 | |
| if start < 1 or num_chars < 0: | |
| return "#VALUE!" |
There was a problem hiding this comment.
Fixed in 73831a1. MID now returns #VALUE\! for start < 1 or num_chars < 0. Two edge case tests added.
|
|
||
|
|
There was a problem hiding this comment.
Missing test coverage for POWER function with negative base and fractional exponent, which would cause a ValueError in the current implementation. This edge case should be tested to verify the expected error handling behavior.
| def test_negative_base_fractional_exponent(self) -> None: | |
| wb = wolfxl.Workbook() | |
| ws = wb.active | |
| ws["A1"] = -4 | |
| ws["B1"] = "=POWER(A1,0.5)" | |
| ev = WorkbookEvaluator() | |
| ev.load(wb) | |
| with pytest.raises(ValueError): | |
| ev.calculate() |
There was a problem hiding this comment.
Fixed in 73831a1. Added POWER(-1, 0.5) test that asserts #NUM\! return.
![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: 83aede1f2e
ℹ️ 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".
| results = ev.calculate() | ||
| # Expected: ~-1073.64 | ||
| val = results["Sheet!A1"] | ||
| assert val is not None, "PMT formula returned None - formulas lib not available?" |
There was a problem hiding this comment.
These tests unconditionally assert that formulas-backed functions return non-None, but the default CI job installs wolfxl without extras (.github/workflows/ci.yml installs wolfxl, not wolfxl[calc]), so formulas is absent and this suite fails even when core code is correct. Please guard this module (or the formulas-only cases) with pytest.importorskip("formulas") or an equivalent marker so the default test matrix remains green.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 73831a1. Added @_requires_formulas skip marker to 4 formulas-dependent test classes. CI now passes without wolfxl[calc] extra.
| nums = _coerce_numeric(args) | ||
| if len(nums) != 2: | ||
| raise ValueError("POWER: non-numeric argument") | ||
| return nums[0] ** nums[1] |
There was a problem hiding this comment.
POWER currently returns nums[0] ** nums[1] directly, which yields a complex number for cases like POWER(-1, 0.5). The evaluator and protocol are built around scalar Excel-like values (int/float/str/bool/None), so storing a complex here can silently break downstream formulas (e.g., arithmetic paths that only accept int|float will start returning None). This should be converted to an Excel-style error path (e.g., raise and surface #NUM!) instead of returning complex values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 73831a1. POWER now guards against complex results and returns #NUM\!.
…CH, XLOOKUP, CHOOSE, SUMIF, SUMIFS, COUNTIF, COUNTIFS) Adds 8 new builtin functions for lookup and conditional aggregation, bringing the builtin count from 25 to 33 and the whitelist from 39 to 43. These are the most commonly used Excel functions in financial models and are required for LRBench-Agent formula evaluation. Key changes: - RangeValue dataclass: shape-aware 2D range container that preserves (n_rows, n_cols) metadata while staying iterable for backward compat - & string concatenation operator: enables dynamic criteria like ">"&B1 - Criteria matching engine: shared by SUMIF/SUMIFS/COUNTIF/COUNTIFS, supports operators (>100), wildcards (a*), and exact match - 43 new tests covering all builtins, backward compat, and perturbation propagation through INDEX/MATCH and SUMIF chains Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
wolfiesch
changed the title
- POWER: return #NUM! for negative base with fractional exponent instead of leaking complex numbers (Copilot #2, Codex #11) - LEFT: return #VALUE! for negative num_chars (Copilot #1) - MID: return #VALUE! for start < 1 or num_chars < 0 (Copilot #8) - range_shape: use min/max instead of abs() for consistency with expand_range (Copilot #3) - Add @_requires_formulas skip marker to formulas-dependent test classes so CI passes without wolfxl[calc] extra (Codex #10) - Add 4 edge case tests: POWER(-1,0.5), LEFT(-1), MID(0,..), MID(.,-1) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Commit 1: formulas library fallback + 10 builtins
formulaslibrary as fallback for functions not in builtins (PMT, SLN, NPV, VLOOKUP, etc.)range_shape()utility for 2D range metadataCommit 2: 8 lookup & conditional aggregation builtins
(n_rows, n_cols)while staying iterable for backward compat&string concatenation operator: enables=SUMIF(A1:A5, ">"&B1, C1:C5)dynamic criteriaWhy
These functions are required for LRBench-Agent scoring where agents build spreadsheets with formulas. The nested
INDEX(range, MATCH(...))pattern is ubiquitous in financial models and needs both functions as builtins.Test plan
&operator, criteria engine, RangeValue backward compat, and perturbation propagation🤖 Generated with Claude Code