fix: dimension-aware effect-total consistency check#745
Conversation
The post-solve validation in _create_effects_dataset compared ds[effect].sum(...) against solution[label] via np.allclose on the raw .values. When the two arrays carry the same dims in a different order -- e.g. a clustered system expanded back yields (scenario, period) while the computed total is (period, scenario) -- numpy cannot broadcast (3,2) against (2,3) and raises ValueError, turning a soft warning into a hard crash. Align the two label-aware before comparing: warn on a genuine dimension-set mismatch, otherwise transpose solution[label] to the computed dim order. Applies to both copies of the check (StatisticsAccessor and Results). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 39 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 Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughEffect-total validation now checks dimension sets and aligns dimension order before comparing computed and stored values. New parametrized regression tests cover transposed solution dimensions across temporal, periodic, and total dataset modes. ChangesEffects dataset validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_effects_dataset_validation.py (1)
36-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the legacy
Resultsvalidator too.This regression test exercises only
fs.stats._create_effects_dataset, whileflixopt/results.pycontains a separately changed implementation. Add an equivalent case forResults._create_effects_datasetso that path cannot regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_effects_dataset_validation.py` around lines 36 - 53, Extend test_effects_dataset_tolerates_transposed_solution_dims to also construct the legacy Results path and invoke Results._create_effects_dataset with the transposed solution dimensions. Assert that the resulting dataset contains costs, matching the existing fs.stats validation coverage while reusing the same mode and reordered-dimension scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@flixopt/statistics_accessor.py`:
- Around line 938-947: In the comparison paths around the statistics accessor
dimension check and the corresponding results comparison, align the found array
to computed.dims, compare computed.sizes with the aligned array’s sizes, and log
a size mismatch before calling np.allclose. Apply this in both
flixopt/statistics_accessor.py:938-947 and flixopt/results.py:969-978,
preserving the existing dimension-mismatch and value-comparison behavior while
preventing incompatible shapes from reaching np.allclose.
---
Nitpick comments:
In `@tests/test_effects_dataset_validation.py`:
- Around line 36-53: Extend
test_effects_dataset_tolerates_transposed_solution_dims to also construct the
legacy Results path and invoke Results._create_effects_dataset with the
transposed solution dimensions. Assert that the resulting dataset contains
costs, matching the existing fs.stats validation coverage while reusing the same
mode and reordered-dimension scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fd62b10-3696-45ed-a216-695d4ff1901c
📒 Files selected for processing (3)
flixopt/results.pyflixopt/statistics_accessor.pytests/test_effects_dataset_validation.py
| if set(computed.dims) != set(found.dims): | ||
| logger.critical( | ||
| f'Results for {effect}({mode}) in effects_dataset doesnt match {label}: ' | ||
| f'dimension mismatch {computed.dims=} vs {found.dims=}' | ||
| ) | ||
| elif not np.allclose( | ||
| computed.fillna(0).values, | ||
| found.transpose(*computed.dims).fillna(0).values, | ||
| equal_nan=True, | ||
| ): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '^(flixopt/statistics_accessor.py|flixopt/results.py)$' || true
echo "== stats_accessor relevant lines =="
if [ -f flixopt/statistics_accessor.py ]; then
sed -n '900,970p' flixopt/statistics_accessor.py | cat -n | sed 's/^/stats_accessor: /'
fi
echo "== results relevant lines =="
if [ -f flixopt/results.py ]; then
sed -n '930,990p' flixopt/results.py | cat -n | sed 's/^/results: /'
fi
echo "== search comparison helper occurrences =="
rg -n "computed\.dims|found\.transpose|np\.allclose|fillna\(0\)\.values|sizes" flixopt/statistics_accessor.py flixopt/results.py || true
echo "== probe xarray dimension-name equality vs size broadcasting with numpy allclose =="
python3 - <<'PY'
import numpy as np
try:
import xarray as xr
a = xr.DataArray(np.arange(3*4), dims=['x','y'], coords={'x':[1,2,3],'y':[1,2,3,4]}).fillna(np.nan)
b = xr.DataArray(np.arange(3*5), dims=['x','y'], coords={'x':[1,2,3],'y':[1,2,3,4,5]}).fillna(np.nan)
print("xarray available", xr.__version__)
print("dims equal sets:", set(a.dims) == set(b.dims))
try:
np.allclose(a.fillna(0).values, b.transpose(*a.dims).fillna(0).values, equal_nan=True)
print("allclose returned")
except Exception as e:
print("allclose exception:", type(e).__name__, str(e))
except Exception as e:
print("xarray probe unavailable:", type(e).__name__, str(e))
PYRepository: flixOpt/flixopt
Length of output: 13925
🌐 Web query:
NumPy allclose ValueError arrays not same shape documentation
💡 Result:
The function numpy.allclose does not raise a ValueError when provided with arrays of different shapes [1][2]. Instead, it uses standard NumPy broadcasting rules to perform the comparison [1][3]. Because of this, numpy.allclose can return True for arrays with different shapes if they are broadcast-compatible and their elements satisfy the tolerance criteria [1][4]. If your goal is to require that both the shapes and the values match, you should use numpy.testing.assert_allclose instead [5][3]. Unlike numpy.allclose, numpy.testing.assert_allclose explicitly checks that the shapes of the two inputs are the same and raises an AssertionError if they do not match [5][3].
Citations:
- 1: https://numpy.org/doc/stable/reference/generated/numpy.allclose.html
- 2: https://numpy.org/doc/2.1/reference/generated/numpy.allclose.html
- 3: https://stackoverflow.com/questions/32874840/test-if-two-numpy-arrays-are-close-to-equal-including-shape
- 4: https://numpy.org/doc/1.20/reference/generated/numpy.allclose.html
- 5: https://numpy.org/doc/stable/reference/generated/numpy.testing.assert_allclose.html
🌐 Web query:
NumPy ufunc broadcasting rules incompatible shapes ValueError shape
💡 Result:
The ValueError: operands could not be broadcast together with shapes is raised when NumPy attempts to perform element-wise operations (via universal functions or ufuncs) on arrays that do not satisfy the required broadcasting compatibility rules [1][2][3]. Broadcasting Rules To determine if two arrays are compatible, NumPy compares their shapes element-wise, starting from the trailing (rightmost) dimension and moving backward toward the leftmost dimension [1][4][5]. Two dimensions are compatible if: 1. They are equal, or 2. One of them is 1 [1][6][5]. If the arrays have a different number of dimensions, the shape of the array with fewer dimensions is prepended with 1s until the number of dimensions matches [6][5]. If at any point the trailing dimensions are unequal and neither is 1, NumPy raises the ValueError [1][5]. Common Causes and How to Resolve This error typically occurs when the dimensions do not align for the intended element-wise operation [7][8]. To fix it, you must align the shapes by: 1. Adding dimensions: Use np.newaxis or None to convert a 1D array of shape (N,) into a column vector of shape (N, 1), which can then broadcast across a 2D array [8][5]. 2. Reshaping: Explicitly use.reshape to match the required dimensions [8][5]. 3. Reordering: If dimensions are swapped (e.g., trying to broadcast a (N, 1) array where a (1, N) is expected), use.transpose or.moveaxis to reorder them [8]. 4. Verifying Operation: If you intended to perform matrix multiplication instead of element-wise broadcasting, use the @ operator or np.matmul instead of standard arithmetic operators [7][8]. Top Results: [1], [8], [5]
Citations:
- 1: https://numpy.org/doc/stable/user/basics.broadcasting.html
- 2: https://numpy.org/doc/stable/user/basics.ufuncs.html
- 3: https://numpy.org/doc/stable/reference/ufuncs.html
- 4: https://numpy.org/doc/2.5/user/basics.broadcasting.html
- 5: https://stackoverflow.com/questions/79698196/numpy-broadcasting-error-when-trying-to-add-2d-array-and-1d-array
- 6: https://numpy.org/doc/2.3/user/basics.broadcasting.html
- 7: https://stackoverflow.com/questions/24560298/python-numpy-valueerror-operands-could-not-be-broadcast-together-with-shapes
- 8: https://thelinuxcode.com/how-to-fix-valueerror-operands-could-not-be-broadcast-together-with-shapes-in-numpy/
🌐 Web query:
NumPy allclose implementation calls subtract absolute less than
💡 Result:
The NumPy implementation of allclose uses the following element-wise comparison to determine if two arrays are close [1][2]: absolute(a - b) <= (atol + rtol * absolute(b)) This implementation computes the absolute difference between input arrays a and b and compares it against the sum of the absolute tolerance (atol) and the scaled relative tolerance (rtol * absolute(b)) [1]. Because this equation uses the absolute value of b, it is asymmetric, meaning allclose(a, b) can yield a different result than allclose(b, a) in some cases [1][2]. This logic aligns with the core requirement of checking if the difference between two values is less than or equal to a combined absolute and relative threshold [1][3]. The function is designed to handle floating-point comparisons where absolute tolerance provides a floor for small values, while relative tolerance scales with the magnitude of the reference value b [1][4].
Citations:
- 1: https://numpy.org/doc/stable/reference/generated/numpy.allclose.html
- 2: https://numpy.org/doc/2.4/reference/generated/numpy.allclose.html
- 3: https://numpy.org/doc/2.3/reference/generated/numpy.allclose.html
- 4: https://stackoverflow.com/questions/65909842/what-is-the-point-of-rtol-in-numpys-allclose-function
Guard incompatible comparison sizes instead of relying on np.allclose.
These checks only ensure dimension names match; if computed and solution[label] have the same dim set but incompatible sizes, np.allclose(..., equal_nan=True) will raise for incompatible shapes and for nan=True implementations before logging. Align found, compare computed.sizes with the aligned array, and log size mismatches before the value comparison in both affected paths.
📍 Affects 2 files
flixopt/statistics_accessor.py#L938-L947(this comment)flixopt/results.py#L969-L978
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@flixopt/statistics_accessor.py` around lines 938 - 947, In the comparison
paths around the statistics accessor dimension check and the corresponding
results comparison, align the found array to computed.dims, compare
computed.sizes with the aligned array’s sizes, and log a size mismatch before
calling np.allclose. Apply this in both flixopt/statistics_accessor.py:938-947
and flixopt/results.py:969-978, preserving the existing dimension-mismatch and
value-comparison behavior while preventing incompatible shapes from reaching
np.allclose.
Curated from property-based fuzzing that ran save -> load -> expand over random combinations of periods (0-3), scenarios (0-3), storage (all initial-charge modes), converters and optimize on/off, same-version and cross-version (7.0.0/7.1.0 -> 7.2.3). No round-trip failures were found; these 24 cases pin the representative dimension layouts so the path stays intact, and assert the effect-total validation does not crash on any layout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Problem
The post-solve consistency check in
_create_effects_datasetcomparesds[effect].sum(...)againstsolution[label]usingnp.allcloseon the raw.values:When the two arrays carry the same dims in a different order — e.g. a clustered system expanded back yields
(scenario, period)while the computed total is(period, scenario)— numpy can't broadcast(3,2)against(2,3)and raisesValueErrorbefore the comparison produces a bool. A soft-warning path becomes a hard crash.Fix
Compare dimension-aware: warn on a genuine dimension-set mismatch, otherwise
found.transpose(*computed.dims)before thenp.allclose. Applied to both copies of the check (StatisticsAccessor._create_effects_datasetandResults._create_effects_dataset).Swept the codebase for the same raw-
.valuespattern:modeling._xr_allclose(usesxr.broadcast) andio.pyconstant-detection are already alignment-safe; remainingnp.array_equal/np.iscloseuses are on 1-D coords or scalars.Tests
Adds
tests/test_effects_dataset_validation.py(3 params: temporal/periodic/total). Verified the tests reproduce the exactValueError: operands could not be broadcast together with shapes (3,2) (2,3)without the fix and pass with it.Release note
Also being shipped to the 7.2.x line as 7.2.3. This PR forward-ports the fix to
main.🤖 Generated with Claude Code
Summary by CodeRabbit