Feature/test math+more #600
Merged
Conversation
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
Test Coverage (56 tests):
Multi-Period with Different Time Series
- TestMultiPeriodDifferentTimeSeries - Tests for systems where each period has distinct demand profiles:
- Different cluster assignments per period
- Optimization with period-specific profiles
- Correct expansion mapping per period
- Statistics correctness per period
Extreme Cluster Configurations
- TestExtremeConfigNewCluster - Tests method='new_cluster':
- Captures peak demand days
- Can increase cluster count
- Works with min_value parameter
- TestExtremeConfigReplace - Tests method='replace':
- Maintains requested cluster count
- Works with multi-period systems
- TestExtremeConfigAppend - Tests method='append':
- Combined with segmentation
- Objective preserved after expansion
Combined Multi-Period and Extremes
- TestExtremeConfigMultiPeriod - Extremes with multi-period/scenario:
- Requires preserve_n_clusters=True for multi-period
- Works with periods and scenarios together
- TestMultiPeriodWithExtremes - Combined scenarios:
- Different profiles with extreme capture
- Extremes combined with segmentation
- Independent cluster assignments per period
Multi-Scenario Clustering
- TestMultiScenarioWithClustering - Scenarios with clustering
- TestFullDimensionalClustering - Full (periods + scenarios) combinations
IO Round-Trip
- TestMultiPeriodClusteringIO - Save/load preservation tests
Edge Cases
- TestEdgeCases - Single cluster, many clusters, occurrence sums, mapping validation
* Fix coords concat in comparison.py
* Fix coords concat in comparison.py
* Fix coords concat in comparison.py
* Add 6.0.1 changelog entry
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* Fix coord preservation in Comparison.solution and .inputs
- Apply _extract_nonindex_coords pattern to solution and inputs properties
- Add warning when coordinate mappings conflict during merge
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* Update CHANGELOG.md
* Update CHANGELOG.md
* ⏺ The fix is straightforward — on line 83, mapping.get(dv) returns None for unmapped values. Change it to mapping.get(dv, dv) so unmapped dimension values fall back to
themselves.
⏺ Update(flixopt/comparison.py)
⎿ Added 1 line, removed 1 line
80 for name, (dim, mapping) in merged.items():
81 if dim not in ds.dims:
82 continue
83 - new_coords[name] = (dim, [mapping.get(dv) for dv in ds.coords[dim].values])
83 + new_coords[name] = (dim, [mapping.get(dv, dv) for dv in ds.coords[dim].values])
84
85 return ds.assign_coords(new_coords)
86
⏺ Done. The change on line 83 ensures that when mapping doesn't contain a key for a dimension value (which happens with outer-join additions), the original value dv is
preserved instead of inserting None.
* Update Changelog
---------
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This reverts commit 4a57282.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis PR prepares a major release with significant API changes—renaming public accessors (statistics→stats), improving non-index coordinate handling in comparisons, tightening clustering validation for multi-period systems, and adding extensive new test suites for clustering, components, scenarios, and storage. Includes dependency updates and stale test refactoring. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…ustering, and validation - Add 26 new tests across 8 files (×3 optimize modes = ~75 test runs) - Multi-period: period weights, flow_hours limits, effect limits, linked invest, custom period weights - Scenarios: scenario weights, independent sizes, independent flow rates - Clustering: basic objective, storage cyclic/intercluster modes, status cyclic mode - Storage: relative min/max charge state, relative min/max final charge state, balanced invest - Components: transmission startup cost, Power2Heat, HeatPumpWithSource, SourceAndSink - Flow status: max_uptime standalone test - Validation: SourceAndSink requires size with prevent_simultaneous
FBumann
force-pushed
the
feature/test-math+more
branch
from
8915cd9 to
ae6afb6
Compare
Fix (flixopt/components.py:1146-1169): In _relative_charge_state_bounds, the scalar else branches now expand the base parameter to regular timesteps only (timesteps_extra[:-1]), then concat with the final-timestep DataArray containing the correct override value. Previously they just broadcast the scalar across all timesteps, silently ignoring relative_minimum_final_charge_state / relative_maximum_final_charge_state. Tests (tests/test_math/test_storage.py): Added two new tests — test_storage_relative_minimum_final_charge_state_scalar and test_storage_relative_maximum_final_charge_state_scalar — identical scenarios to the existing array-based tests but using scalar defaults (the previously buggy path).
…estep values in clustered systems: 1. test_flow_rates_match_demand_per_cluster — Verifies Grid flow_rate matches demand [10,20,30,40] identically in each cluster, objective = 200. 2. test_per_timestep_effects_with_varying_price — Verifies per-timestep costs [10,20,30,40] reflect price×flow with varying prices [1,2,3,4] and constant demand=10, objective = 200. 3. test_storage_cyclic_charge_discharge_pattern — Verifies storage with cyclic clustering: charges at cheap timesteps (price=1), discharges at expensive ones (price=100), with exact charge_state trajectory across both clusters, objective = 100. Deviation from plan: Used equal cluster weights [1.0, 1.0] instead of [1.0, 2.0]/[1.0, 3.0] for tests 1 and 2. This was necessary because cluster_weight is not preserved during NetCDF roundtrip (pre-existing IO bug), which would cause the save->reload->solve mode to fail. Equal weights produce correct results in all 3 IO modes while still testing the essential per-timestep value correctness.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 332-346: The fenced Python code blocks in CHANGELOG.md should be
converted to indented code blocks to satisfy MD046: replace the triple-backtick
fenced blocks containing the add_constraints function and its calls (references:
add_constraints, flow_system.optimize, flow_system.optimize.rolling_horizon)
with a consistently indented block (four spaces per code line) so the code is
rendered as an indented code block; apply the same transformation to the other
occurrences mentioned (around the before_solve, cluster_mode, from_old_dataset
examples at the other ranges).
In `@docs/notebooks/08d-clustering-multiperiod.ipynb`:
- Line 528: The markdown cell that starts with the literal string
"<cell_type>markdown</cell_type>## Summary" contains stray XML/HTML-like markup
and should be cleaned so the cell contains just the markdown header "##
Summary"; remove the "<cell_type>markdown</cell_type>" token from that notebook
markdown cell to prevent it rendering as literal text.
In `@docs/notebooks/08f-clustering-segmentation.ipynb`:
- Line 530: Remove the accidental literal marker
"<cell_type>markdown</cell_type>" from the markdown cell so the cell reads
cleanly (e.g., start with "## API Reference"); locate the markdown cell
containing the string "<cell_type>markdown</cell_type>" and delete that
substring so only the intended header and content remain.
In `@flixopt/comparison.py`:
- Around line 32-71: The function _extract_nonindex_coords silently keeps the
first dimension when the same coord name appears on different dims; update the
loop that builds merged to detect if name is already in merged with a different
dim (compare dim vs merged[name][0]) and in that case emit a warnings.warn
mentioning the coord name and both dims, add the coord name to coords_to_drop to
avoid applying a mismatched mapping, and skip merging values for that coord (do
not overwrite merged entry); this prevents silent misapplication of coord
mappings while still dropping the problematic coord from datasets.
In `@tests/test_clustering/test_multiperiod_extremes.py`:
- Around line 991-1008: The assertions in test_timestep_mapping_valid_range use
xarray DataArray results (mapping.min(), mapping.max(), period_mapping.min(),
period_mapping.max()) which return DataArray objects and cause ambiguous
truth-value errors; update the comparisons to extract scalars via .item() (e.g.,
mapping.min().item(), mapping.max().item(), period_mapping.min().item(),
period_mapping.max().item()) before comparing to 0 and max_valid so the
assertions operate on plain Python numbers.
In `@tests/test_math/test_flow_status.py`:
- Around line 440-507: The test's expected cost in test_max_uptime_standalone is
inconsistent with the docstring and arithmetic (cheap covers 4×10 fuel -> 40
fuel, backup covers 10/0.5 -> 20 fuel, total 60), so update the assertion on
fs.solution['costs'] to require it be approximately 60 (e.g. > 60.0 - small
epsilon or assert near-equality) instead of the loose >50.0; also keep the
existing status check for CheapBoiler(heat)|status to ensure max_uptime
enforcement.
🧹 Nitpick comments (1)
tests/test_clustering/test_multiperiod_extremes.py (1)
692-713: The code works correctly as-is with xarray >= 2024.2.0 and does not raise errors. When.min()/.max()are called on a 1D DataArray (result of.sel(period=period)), they return 0-dimensional DataArrays. Comparisons with scalars (e.g.,>= 0,< 3) produce 0-d boolean DataArrays that work fine in assert statements. The "truth value is ambiguous" error only occurs with multi-element arrays, not 0-d results.While using
.item()is a valid best practice for explicit scalar conversion and improved code clarity, it is not required for the current implementation. The existing code is functionally correct.
Comment on lines
+332
to
+346
| ```python | ||
| def add_constraints(fs): | ||
| model = fs.model | ||
| boiler = model.variables['Boiler(Q_th)|flow_rate'] | ||
| model.add_constraints(boiler >= 10, name='min_boiler') | ||
|
|
||
| flow_system.optimize(solver, before_solve=add_constraints) | ||
|
|
||
| # Works with rolling_horizon too | ||
| flow_system.optimize.rolling_horizon( | ||
| solver, | ||
| horizon=168, | ||
| before_solve=add_constraints | ||
| ) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Fix MD046: use indented code blocks instead of fenced blocks.
markdownlint expects indented blocks in these sections (before_solve, cluster_mode, from_old_dataset). Please convert the fenced blocks accordingly.
🛠️ Sample fix (apply similarly to the other blocks)
-```python
-def add_constraints(fs):
- model = fs.model
- boiler = model.variables['Boiler(Q_th)|flow_rate']
- model.add_constraints(boiler >= 10, name='min_boiler')
-
-flow_system.optimize(solver, before_solve=add_constraints)
-
-# Works with rolling_horizon too
-flow_system.optimize.rolling_horizon(
- solver,
- horizon=168,
- before_solve=add_constraints
-)
-```
+ def add_constraints(fs):
+ model = fs.model
+ boiler = model.variables['Boiler(Q_th)|flow_rate']
+ model.add_constraints(boiler >= 10, name='min_boiler')
+
+ flow_system.optimize(solver, before_solve=add_constraints)
+
+ # Works with rolling_horizon too
+ flow_system.optimize.rolling_horizon(
+ solver,
+ horizon=168,
+ before_solve=add_constraints
+ )Also applies to: 352-358, 376-378
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 332-332: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 Prompt for AI Agents
In `@CHANGELOG.md` around lines 332 - 346, The fenced Python code blocks in
CHANGELOG.md should be converted to indented code blocks to satisfy MD046:
replace the triple-backtick fenced blocks containing the add_constraints
function and its calls (references: add_constraints, flow_system.optimize,
flow_system.optimize.rolling_horizon) with a consistently indented block (four
spaces per code line) so the code is rendered as an indented code block; apply
the same transformation to the other occurrences mentioned (around the
before_solve, cluster_mode, from_old_dataset examples at the other ranges).
| "metadata": {}, | ||
| "source": [ | ||
| "## Summary\n", | ||
| "<cell_type>markdown</cell_type>## Summary\n", |
Contributor
There was a problem hiding this comment.
Remove stray <cell_type> markup from markdown content.
It will render as literal text in the notebook.
✅ Suggested fix
- "<cell_type>markdown</cell_type>## Summary\n",
+ "## Summary\n",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "<cell_type>markdown</cell_type>## Summary\n", | |
| "## Summary\n", |
🤖 Prompt for AI Agents
In `@docs/notebooks/08d-clustering-multiperiod.ipynb` at line 528, The markdown
cell that starts with the literal string "<cell_type>markdown</cell_type>##
Summary" contains stray XML/HTML-like markup and should be cleaned so the cell
contains just the markdown header "## Summary"; remove the
"<cell_type>markdown</cell_type>" token from that notebook markdown cell to
prevent it rendering as literal text.
| "metadata": {}, | ||
| "source": [ | ||
| "## API Reference\n", | ||
| "<cell_type>markdown</cell_type>## API Reference\n", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the notebook file size and structure around line 530
wc -l docs/notebooks/08f-clustering-segmentation.ipynb
echo "---"
# Show context around line 530
sed -n '525,535p' docs/notebooks/08f-clustering-segmentation.ipynbRepository: flixOpt/flixopt
Length of output: 372
🏁 Script executed:
#!/bin/bash
# Check if this is a Jupyter notebook JSON structure
head -20 docs/notebooks/08f-clustering-segmentation.ipynb | head -c 200
echo "---"
# Look for the actual string in context
grep -n "<cell_type>markdown</cell_type>" docs/notebooks/08f-clustering-segmentation.ipynb | head -5Repository: flixOpt/flixopt
Length of output: 323
🏁 Script executed:
#!/bin/bash
# Search for build scripts that might process notebooks
find . -type f \( -name "*.py" -o -name "*.sh" -o -name "Makefile" -o -name "*.toml" -o -name "*.yaml" -o -name "*.yml" \) | grep -E "(build|process|notebook|docs)" | head -20Repository: flixOpt/flixopt
Length of output: 307
🏁 Script executed:
#!/bin/bash
# Check the docs build configuration
cat .github/workflows/docs.yaml
echo "---"
# Check mkdocs.yml for any notebook processing
cat mkdocs.ymlRepository: flixOpt/flixopt
Length of output: 16642
🏁 Script executed:
#!/bin/bash
# Look for any notebook processing in Python files
grep -r "<cell_type>" docs/notebooks/data/*.py
echo "---"
# Check if there's any processing of notebook source in build scripts
grep -r "cell_type\|<cell_type>" --include="*.py" | grep -v ".ipynb" | head -10Repository: flixOpt/flixopt
Length of output: 62
🏁 Script executed:
#!/bin/bash
# See the full context of that cell to understand better
sed -n '527,535p' docs/notebooks/08f-clustering-segmentation.ipynbRepository: flixOpt/flixopt
Length of output: 283
🏁 Script executed:
#!/bin/bash
# Check if there's any processing that might strip <cell_type> markers
grep -r "cell_type" docs/ --include="*.py" --include="*.md" | grep -v ".ipynb"
echo "---"
# Look for any custom notebook processing
find scripts -name "*.py" -exec basename {} \;Repository: flixOpt/flixopt
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Check the actual rendering to see if literal text appears
# Look for any configuration that might filter notebook cell sources
grep -r "include_source\|filter\|strip" mkdocs.yml
echo "---"
# See if there are other instances of this pattern that might indicate intent
grep -c "<cell_type>" docs/notebooks/08f-clustering-segmentation.ipynbRepository: flixOpt/flixopt
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Check if mkdocs-jupyter has any built-in filtering of cell source
# First find where mkdocs-jupyter might be configured or documented
pip show mkdocs-jupyter 2>/dev/null | grep -E "Location|Version"
echo "---"
# Check if <cell_type> looks like an intentional pattern or accidental
# by looking at commit history or comments near line 530
sed -n '520,540p' docs/notebooks/08f-clustering-segmentation.ipynb | cat -ARepository: flixOpt/flixopt
Length of output: 767
🏁 Script executed:
#!/bin/bash
# Let's check if this pattern exists elsewhere in the repo
find . -name "*.ipynb" -exec grep -l "<cell_type>" {} \;Repository: flixOpt/flixopt
Length of output: 197
🏁 Script executed:
#!/bin/bash
# Check if there's any special handling or if this is documented as intentional
grep -B5 -A5 "<cell_type>markdown</cell_type>" docs/notebooks/08f-clustering-segmentation.ipynb | head -20Repository: flixOpt/flixopt
Length of output: 315
🏁 Script executed:
#!/bin/bash
# Let's understand the actual rendered output by checking if there's a way to see what the docs look like
# First check if site/ directory exists (built docs)
ls -la site/notebooks/08f* 2>/dev/null || echo "No built site found"
echo "---"
# Check if there's a way to see what mkdocs-jupyter outputs
python3 << 'EOF'
import json
# Load the notebook
with open('docs/notebooks/08f-clustering-segmentation.ipynb', 'r') as f:
nb = json.load(f)
# Find the cell with <cell_type>markdown
for i, cell in enumerate(nb['cells']):
if cell.get('cell_type') == 'markdown':
source = ''.join(cell.get('source', []))
if '<cell_type>markdown</cell_type>' in source:
print(f"Cell {i}: cell_type='{cell['cell_type']}'")
print(f"First 150 chars of source:\n{source[:150]}")
print("---")
EOFRepository: flixOpt/flixopt
Length of output: 296
🏁 Script executed:
#!/bin/bash
# Verify this appears in the actual rendered markdown by simulating what mkdocs-jupyter outputs
python3 << 'EOF'
import json
# Simulate how mkdocs-jupyter would process the cell
with open('docs/notebooks/08f-clustering-segmentation.ipynb', 'r') as f:
nb = json.load(f)
for cell in nb['cells']:
if cell.get('cell_type') == 'markdown' and '<cell_type>markdown</cell_type>' in ''.join(cell.get('source', [])):
print("=== RENDERED OUTPUT ===")
print("When mkdocs-jupyter processes this cell, it will output:")
print()
source_text = ''.join(cell.get('source', []))
print(source_text[:200])
print()
print("The literal '<cell_type>markdown</cell_type>' text will appear in the HTML output.")
break
EOFRepository: flixOpt/flixopt
Length of output: 427
Remove the literal <cell_type>markdown</cell_type> marker from the markdown cell source.
This string renders as literal text in the generated documentation. It appears to be accidental markup and should be deleted from line 530.
🤖 Prompt for AI Agents
In `@docs/notebooks/08f-clustering-segmentation.ipynb` at line 530, Remove the
accidental literal marker "<cell_type>markdown</cell_type>" from the markdown
cell so the cell reads cleanly (e.g., start with "## API Reference"); locate the
markdown cell containing the string "<cell_type>markdown</cell_type>" and delete
that substring so only the intended header and content remain.
Comment on lines
32
to
71
| def _extract_nonindex_coords(datasets: list[xr.Dataset]) -> tuple[list[xr.Dataset], dict[str, tuple[str, dict]]]: | ||
| """Extract and merge non-index coords, returning cleaned datasets and merged mappings. | ||
|
|
||
| Non-index coords (like `component` on `contributor` dim) cause concat conflicts. | ||
| This extracts them, merges the mappings, and returns datasets without them. | ||
| """ | ||
| if not datasets: | ||
| return datasets, {} | ||
|
|
||
| # Find non-index coords and collect mappings | ||
| merged: dict[str, tuple[str, dict]] = {} | ||
| coords_to_drop: set[str] = set() | ||
|
|
||
| for ds in datasets: | ||
| for name, coord in ds.coords.items(): | ||
| if len(coord.dims) != 1: | ||
| continue | ||
| dim = coord.dims[0] | ||
| if dim == name or dim not in ds.coords: | ||
| continue | ||
|
|
||
| coords_to_drop.add(name) | ||
| if name not in merged: | ||
| merged[name] = (dim, {}) | ||
|
|
||
| for dv, cv in zip(ds.coords[dim].values, coord.values, strict=False): | ||
| if dv not in merged[name][1]: | ||
| merged[name][1][dv] = cv | ||
| elif merged[name][1][dv] != cv: | ||
| warnings.warn( | ||
| f"Coordinate '{name}' has conflicting values for dim value '{dv}': " | ||
| f"'{merged[name][1][dv]}' vs '{cv}'. Keeping first value.", | ||
| stacklevel=4, | ||
| ) | ||
|
|
||
| # Drop these coords from datasets | ||
| if coords_to_drop: | ||
| datasets = [ds.drop_vars(coords_to_drop, errors='ignore') for ds in datasets] | ||
|
|
||
| return datasets, merged |
Contributor
There was a problem hiding this comment.
Warn when the same non‑index coord name binds to different dims.
If a later dataset has the same coord name on a different dimension, the merge silently keeps the first dim and the coord can be dropped or misapplied. Consider detecting this and warning/short‑circuiting to avoid silent data loss.
⚠️ Possible guard to surface the mismatch
if name not in merged:
merged[name] = (dim, {})
+ elif merged[name][0] != dim:
+ warnings.warn(
+ f"Coordinate '{name}' appears on multiple dims "
+ f"({merged[name][0]} vs {dim}). Keeping the first.",
+ stacklevel=4,
+ )
+ continue🤖 Prompt for AI Agents
In `@flixopt/comparison.py` around lines 32 - 71, The function
_extract_nonindex_coords silently keeps the first dimension when the same coord
name appears on different dims; update the loop that builds merged to detect if
name is already in merged with a different dim (compare dim vs merged[name][0])
and in that case emit a warnings.warn mentioning the coord name and both dims,
add the coord name to coords_to_drop to avoid applying a mismatched mapping, and
skip merging values for that coord (do not overwrite merged entry); this
prevents silent misapplication of coord mappings while still dropping the
problematic coord from datasets.
Comment on lines
991
to
1008
| def test_timestep_mapping_valid_range(self, timesteps_8_days, periods_2): | ||
| """Test that timestep_mapping values are within valid range.""" | ||
| fs = create_multiperiod_system_with_different_profiles(timesteps_8_days, periods_2) | ||
|
|
||
| fs_clustered = fs.transform.cluster(n_clusters=3, cluster_duration='1D') | ||
|
|
||
| mapping = fs_clustered.clustering.timestep_mapping | ||
|
|
||
| # Mapping values should be in [0, n_clusters * timesteps_per_cluster - 1] | ||
| max_valid = 3 * 24 - 1 # n_clusters * timesteps_per_cluster - 1 | ||
| assert mapping.min() >= 0 | ||
| assert mapping.max() <= max_valid | ||
|
|
||
| # Each period should have valid mappings | ||
| for period in periods_2: | ||
| period_mapping = mapping.sel(period=period) | ||
| assert period_mapping.min() >= 0 | ||
| assert period_mapping.max() <= max_valid |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine the test file
find . -name "test_multiperiod_extremes.py" -type fRepository: flixOpt/flixopt
Length of output: 111
🏁 Script executed:
# Get a sense of the file structure
git ls-files tests/test_clustering/ | head -20Repository: flixOpt/flixopt
Length of output: 220
🏁 Script executed:
# Read the specific test method
sed -n '991,1008p' tests/test_clustering/test_multiperiod_extremes.pyRepository: flixOpt/flixopt
Length of output: 911
🏁 Script executed:
# Check what type clustering.timestep_mapping is by examining the clustering module
find . -name "*.py" -type f | xargs grep -l "timestep_mapping" | grep -v test | head -5Repository: flixOpt/flixopt
Length of output: 87
🏁 Script executed:
# Find where timestep_mapping is defined
grep -n "timestep_mapping" ./flixopt/clustering/base.py | head -10Repository: flixOpt/flixopt
Length of output: 768
🏁 Script executed:
# Read the clustering base module to understand the structure
wc -l ./flixopt/clustering/base.pyRepository: flixOpt/flixopt
Length of output: 92
🏁 Script executed:
# Get context around timestep_mapping definition
grep -B 5 -A 10 "def timestep_mapping" ./flixopt/clustering/base.pyRepository: flixOpt/flixopt
Length of output: 669
🏁 Script executed:
# Test xarray behavior with min/max and scalar comparisons
python3 << 'EOF'
import xarray as xr
import numpy as np
# Create a sample DataArray similar to what timestep_mapping would be
data = np.array([[0, 1, 2], [3, 4, 5]])
da = xr.DataArray(data, dims=['period', 'time'], coords={'period': [1, 2], 'time': [0, 1, 2]})
print("Testing xarray.DataArray behavior:")
print("=" * 50)
# Test 1: Check return type of min() and max()
min_result = da.min()
max_result = da.max()
print(f"Type of da.min(): {type(min_result)}")
print(f"Value of da.min(): {min_result}")
print(f"Type of da.max(): {type(max_result)}")
print(f"Value of da.max(): {max_result}")
print("\n" + "=" * 50)
print("Testing direct comparisons:")
print("=" * 50)
# Test 2: Try direct comparisons
try:
result1 = da.min() >= 0
print(f"da.min() >= 0 works: {result1}")
print(f"Type of result: {type(result1)}")
except Exception as e:
print(f"da.min() >= 0 raises error: {type(e).__name__}: {e}")
# Test 3: Try in assertion
try:
assert da.min() >= 0
print("assert da.min() >= 0 works!")
except Exception as e:
print(f"assert da.min() >= 0 raises error: {type(e).__name__}: {e}")
print("\n" + "=" * 50)
print("Testing with .item():")
print("=" * 50)
# Test 4: Using .item()
try:
result2 = da.min().item() >= 0
print(f"da.min().item() >= 0 works: {result2}")
print(f"Type of result: {type(result2)}")
except Exception as e:
print(f"da.min().item() >= 0 raises error: {type(e).__name__}: {e}")
# Test 5: Try in assertion with .item()
try:
assert da.min().item() >= 0
print("assert da.min().item() >= 0 works!")
except Exception as e:
print(f"assert da.min().item() >= 0 raises error: {type(e).__name__}: {e}")
print("\n" + "=" * 50)
print("Testing with .values:")
print("=" * 50)
# Test 6: Using .values (alternative approach)
try:
result3 = da.min().values >= 0
print(f"da.min().values >= 0 works: {result3}")
print(f"Type of result: {type(result3)}")
except Exception as e:
print(f"da.min().values >= 0 raises error: {type(e).__name__}: {e}")
# Test 7: One-element DataArray behavior (like period_mapping selection)
print("\n" + "=" * 50)
print("Testing period_mapping (selected DataArray):")
print("=" * 50)
period_mapping = da.sel(period=1)
print(f"Type of period_mapping: {type(period_mapping)}")
print(f"Shape of period_mapping: {period_mapping.shape}")
print(f"Type of period_mapping.min(): {type(period_mapping.min())}")
try:
assert period_mapping.min() >= 0
print("assert period_mapping.min() >= 0 works!")
except Exception as e:
print(f"assert period_mapping.min() >= 0 raises error: {type(e).__name__}: {e}")
EOFRepository: flixOpt/flixopt
Length of output: 177
🏁 Script executed:
# Install xarray first
pip install xarray numpy --quietRepository: flixOpt/flixopt
Length of output: 286
🏁 Script executed:
# Now test xarray behavior
python3 << 'EOF'
import xarray as xr
import numpy as np
# Create a sample DataArray similar to what timestep_mapping would be
data = np.array([[0, 1, 2], [3, 4, 5]])
da = xr.DataArray(data, dims=['period', 'time'], coords={'period': [1, 2], 'time': [0, 1, 2]})
print("Testing xarray.DataArray behavior:")
print("=" * 50)
# Test 1: Check return type of min() and max()
min_result = da.min()
max_result = da.max()
print(f"Type of da.min(): {type(min_result)}")
print(f"Value of da.min(): {min_result}")
print(f"Type of da.max(): {type(max_result)}")
print(f"Value of da.max(): {max_result}")
print("\n" + "=" * 50)
print("Testing direct comparisons:")
print("=" * 50)
# Test 2: Try direct comparisons
try:
result1 = da.min() >= 0
print(f"da.min() >= 0 works: {result1}")
print(f"Type of result: {type(result1)}")
except Exception as e:
print(f"da.min() >= 0 raises error: {type(e).__name__}: {e}")
# Test 3: Try in assertion
try:
assert da.min() >= 0
print("assert da.min() >= 0 works!")
except Exception as e:
print(f"assert da.min() >= 0 raises error: {type(e).__name__}: {e}")
print("\n" + "=" * 50)
print("Testing with .item():")
print("=" * 50)
# Test 4: Using .item()
try:
result2 = da.min().item() >= 0
print(f"da.min().item() >= 0 works: {result2}")
print(f"Type of result: {type(result2)}")
except Exception as e:
print(f"da.min().item() >= 0 raises error: {type(e).__name__}: {e}")
# Test 5: Try in assertion with .item()
try:
assert da.min().item() >= 0
print("assert da.min().item() >= 0 works!")
except Exception as e:
print(f"assert da.min().item() >= 0 raises error: {type(e).__name__}: {e}")
print("\n" + "=" * 50)
print("Testing with .values:")
print("=" * 50)
# Test 6: Using .values (alternative approach)
try:
result3 = da.min().values >= 0
print(f"da.min().values >= 0 works: {result3}")
print(f"Type of result: {type(result3)}")
except Exception as e:
print(f"da.min().values >= 0 raises error: {type(e).__name__}: {e}")
# Test 7: One-element DataArray behavior (like period_mapping selection)
print("\n" + "=" * 50)
print("Testing period_mapping (selected DataArray):")
print("=" * 50)
period_mapping = da.sel(period=1)
print(f"Type of period_mapping: {type(period_mapping)}")
print(f"Shape of period_mapping: {period_mapping.shape}")
print(f"Type of period_mapping.min(): {type(period_mapping.min())}")
try:
assert period_mapping.min() >= 0
print("assert period_mapping.min() >= 0 works!")
except Exception as e:
print(f"assert period_mapping.min() >= 0 raises error: {type(e).__name__}: {e}")
EOFRepository: flixOpt/flixopt
Length of output: 177
🏁 Script executed:
# Check project dependencies
find . -maxdepth 2 -name "setup.py" -o -name "pyproject.toml" -o -name "requirements*.txt" | head -10Repository: flixOpt/flixopt
Length of output: 75
🏁 Script executed:
# Let's check the existing test execution to see if there are actual issues
grep -r "assert.*\.min()" tests/test_clustering/ --include="*.py"Repository: flixOpt/flixopt
Length of output: 339
🏁 Script executed:
# Check if the tests currently pass or fail
find . -name "pytest.ini" -o -name "setup.cfg" -o -name "tox.ini" | head -5Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
# Check pyproject.toml for xarray
cat pyproject.toml | grep -A 20 -B 5 "xarray"Repository: flixOpt/flixopt
Length of output: 1544
🏁 Script executed:
# Check how xarray min/max is used elsewhere in the codebase
grep -r "\.min()" flixopt/ --include="*.py" -A 1 -B 1 | head -40Repository: flixOpt/flixopt
Length of output: 1787
🏁 Script executed:
# Search for how assertions are done with xarray in existing tests
grep -r "assert.*\.min()\|assert.*\.max()" tests/ --include="*.py" -B 2 -A 2 | head -60Repository: flixOpt/flixopt
Length of output: 2535
🏁 Script executed:
# Verify what period_assignments is in the other test to see if there's a pattern difference
grep -B 10 "period_assignments.min()" tests/test_clustering/test_multiperiod_extremes.py | head -20Repository: flixOpt/flixopt
Length of output: 621
🏁 Script executed:
# Check if there's documentation or type hints for these objects
grep -B 5 "period_assignments" tests/test_clustering/test_multiperiod_extremes.py | grep "def\|:" | head -10Repository: flixOpt/flixopt
Length of output: 124
Fix scalar comparisons on xarray results to avoid ambiguous truth values.
mapping.min()/max() and period_mapping.min()/max() return DataArray objects; direct comparisons in assertions raise ambiguity errors. Use .item() to extract scalar values.
Suggested fix
- assert mapping.min() >= 0
- assert mapping.max() <= max_valid
+ assert mapping.min().item() >= 0
+ assert mapping.max().item() <= max_valid
@@
- assert period_mapping.min() >= 0
- assert period_mapping.max() <= max_valid
+ assert period_mapping.min().item() >= 0
+ assert period_mapping.max().item() <= max_valid📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_timestep_mapping_valid_range(self, timesteps_8_days, periods_2): | |
| """Test that timestep_mapping values are within valid range.""" | |
| fs = create_multiperiod_system_with_different_profiles(timesteps_8_days, periods_2) | |
| fs_clustered = fs.transform.cluster(n_clusters=3, cluster_duration='1D') | |
| mapping = fs_clustered.clustering.timestep_mapping | |
| # Mapping values should be in [0, n_clusters * timesteps_per_cluster - 1] | |
| max_valid = 3 * 24 - 1 # n_clusters * timesteps_per_cluster - 1 | |
| assert mapping.min() >= 0 | |
| assert mapping.max() <= max_valid | |
| # Each period should have valid mappings | |
| for period in periods_2: | |
| period_mapping = mapping.sel(period=period) | |
| assert period_mapping.min() >= 0 | |
| assert period_mapping.max() <= max_valid | |
| def test_timestep_mapping_valid_range(self, timesteps_8_days, periods_2): | |
| """Test that timestep_mapping values are within valid range.""" | |
| fs = create_multiperiod_system_with_different_profiles(timesteps_8_days, periods_2) | |
| fs_clustered = fs.transform.cluster(n_clusters=3, cluster_duration='1D') | |
| mapping = fs_clustered.clustering.timestep_mapping | |
| # Mapping values should be in [0, n_clusters * timesteps_per_cluster - 1] | |
| max_valid = 3 * 24 - 1 # n_clusters * timesteps_per_cluster - 1 | |
| assert mapping.min().item() >= 0 | |
| assert mapping.max().item() <= max_valid | |
| # Each period should have valid mappings | |
| for period in periods_2: | |
| period_mapping = mapping.sel(period=period) | |
| assert period_mapping.min().item() >= 0 | |
| assert period_mapping.max().item() <= max_valid |
🤖 Prompt for AI Agents
In `@tests/test_clustering/test_multiperiod_extremes.py` around lines 991 - 1008,
The assertions in test_timestep_mapping_valid_range use xarray DataArray results
(mapping.min(), mapping.max(), period_mapping.min(), period_mapping.max()) which
return DataArray objects and cause ambiguous truth-value errors; update the
comparisons to extract scalars via .item() (e.g., mapping.min().item(),
mapping.max().item(), period_mapping.min().item(), period_mapping.max().item())
before comparing to 0 and max_valid so the assertions operate on plain Python
numbers.
tests/test_math/test_flow_status.py
Outdated
Comment on lines
440
to
507
| def test_max_uptime_standalone(self, optimize): | ||
| """Proves: max_uptime on a flow limits continuous operation, forcing | ||
| the unit to shut down and hand off to a backup. | ||
|
|
||
| CheapBoiler (eta=1.0) with max_uptime=2, previous_flow_rate=0. | ||
| ExpensiveBackup (eta=0.5). Demand=[10]*5. | ||
| Cheap boiler can run at most 2 consecutive hours, then must shut down. | ||
| Pattern: on(0,1), off(2), on(3,4) → cheap covers 4h, backup covers 1h. | ||
|
|
||
| Sensitivity: Without max_uptime, all 5 hours cheap → cost=50. | ||
| With max_uptime=2, backup covers 1 hour at eta=0.5 → cost=70. | ||
| """ | ||
| fs = make_flow_system(5) | ||
| fs.add_elements( | ||
| fx.Bus('Heat'), | ||
| fx.Bus('Gas'), | ||
| fx.Effect('costs', '€', is_standard=True, is_objective=True), | ||
| fx.Sink( | ||
| 'Demand', | ||
| inputs=[ | ||
| fx.Flow( | ||
| 'heat', | ||
| bus='Heat', | ||
| size=1, | ||
| fixed_relative_profile=np.array([10, 10, 10, 10, 10]), | ||
| ), | ||
| ], | ||
| ), | ||
| fx.Source( | ||
| 'GasSrc', | ||
| outputs=[fx.Flow('gas', bus='Gas', effects_per_flow_hour=1)], | ||
| ), | ||
| fx.linear_converters.Boiler( | ||
| 'CheapBoiler', | ||
| thermal_efficiency=1.0, | ||
| fuel_flow=fx.Flow('fuel', bus='Gas'), | ||
| thermal_flow=fx.Flow( | ||
| 'heat', | ||
| bus='Heat', | ||
| size=100, | ||
| previous_flow_rate=0, | ||
| status_parameters=fx.StatusParameters(max_uptime=2), | ||
| ), | ||
| ), | ||
| fx.linear_converters.Boiler( | ||
| 'ExpensiveBackup', | ||
| thermal_efficiency=0.5, | ||
| fuel_flow=fx.Flow('fuel', bus='Gas'), | ||
| thermal_flow=fx.Flow('heat', bus='Heat', size=100), | ||
| ), | ||
| ) | ||
| fs = optimize(fs) | ||
| # CheapBoiler max 2 consecutive hours. Pattern: on,on,off,on,on. | ||
| # Cheap: 4×10 = 40 fuel. Expensive backup @t2: 10/0.5 = 20 fuel. | ||
| # Total = 60. | ||
| # Verify no more than 2 consecutive on-hours | ||
| status = fs.solution['CheapBoiler(heat)|status'].values[:-1] | ||
| max_consecutive = 0 | ||
| current = 0 | ||
| for s in status: | ||
| if s > 0.5: | ||
| current += 1 | ||
| max_consecutive = max(max_consecutive, current) | ||
| else: | ||
| current = 0 | ||
| assert max_consecutive <= 2, f'max_uptime violated: {status}' | ||
| # Cost must be higher than without constraint (50) | ||
| assert fs.solution['costs'].item() > 50.0 + 1e-5 |
Contributor
There was a problem hiding this comment.
Align expected cost and tighten the assertion.
The docstring says cost=70, but the arithmetic here yields 60, and the loose > 50 assertion can pass even if the cheap boiler never runs. Tighten the expectation to catch regressions.
🔧 Suggested fix
- With max_uptime=2, backup covers 1 hour at eta=0.5 → cost=70.
+ With max_uptime=2, backup covers 1 hour at eta=0.5 → cost=60.
@@
- assert fs.solution['costs'].item() > 50.0 + 1e-5
+ assert_allclose(fs.solution['costs'].item(), 60.0, rtol=1e-5)🤖 Prompt for AI Agents
In `@tests/test_math/test_flow_status.py` around lines 440 - 507, The test's
expected cost in test_max_uptime_standalone is inconsistent with the docstring
and arithmetic (cheap covers 4×10 fuel -> 40 fuel, backup covers 10/0.5 -> 20
fuel, total 60), so update the assertion on fs.solution['costs'] to require it
be approximately 60 (e.g. > 60.0 - small epsilon or assert near-equality)
instead of the loose >50.0; also keep the existing status check for
CheapBoiler(heat)|status to ensure max_uptime enforcement.
…ype>markdown</cell_type> from Summary cell 3. 08f-clustering-segmentation.ipynb (cell 33): Removed stray <cell_type>markdown</cell_type> from API Reference cell 4. flixopt/comparison.py: _extract_nonindex_coords now detects when the same coord name appears on different dims — warns and skips instead of silently overwriting 5. test_multiperiod_extremes.py: Added .item() to mapping.min()/.max() and period_mapping.min()/.max() to extract scalars before comparison 6. test_flow_status.py: Tightened test_max_uptime_standalone assertion from > 50.0 to assert_allclose(..., 60.0, rtol=1e-5) matching the documented arithmetic
18 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
tests/test_math/with 25 new analytical correctness tests (×3 optimize modes = 75 test runs)Type of Change
Detailed Plan
Files to modify
tests/test_math/conftest.py— add multi-period and scenario helperstests/test_math/test_storage.py— add 5 teststests/test_math/test_components.py— add 2 + 3 new classestests/test_math/test_flow_status.py— add 1 testFiles to create
tests/test_math/test_multi_period.py— 7 teststests/test_math/test_scenarios.py— 3 teststests/test_math/test_clustering.py— 4 testsAll tests use the existing
optimizefixture (3 modes:solve,save->reload->solve,solve->save->reload).Part A — Single-period gaps
A1. Storage (
test_storage.py, existingTestStorage)test_storage_relative_minimum_charge_statetest_storage_maximal_final_charge_statetest_storage_relative_minimum_final_charge_statetest_storage_relative_maximum_final_charge_statetest_storage_balanced_investA2. Transmission (
test_components.py, existingTestTransmission)test_transmission_prevent_simultaneous_bidirectionaltest_transmission_status_startup_costA3. New component classes (
test_components.py)TestPower2Heat—test_power2heat_efficiencyTestHeatPumpWithSource—test_heatpump_with_source_copTestSourceAndSink—test_source_and_sink_prevent_simultaneousGridConnection: buy @5, sell @-1, prevent_simultaneous=TrueA4. Flow status (
test_flow_status.py)test_max_uptime_standalonePart B — Multi-period, scenarios, clustering
B1. conftest.py helpers
Note: Multi-period objective assertion —
fs.solution['costs'].item()only works for scalar results. For multi-period, need to verify how to access the total objective (e.g.,fs.solution['objective'].item()orfs.model.model.objective.value). Verify during implementation.B2. Multi-period (
test_multi_period.py, newTestMultiPeriod)test_period_weights_affect_objectivetest_flow_hours_max_over_periodstest_flow_hours_min_over_periodstest_effect_maximum_over_periodstest_effect_minimum_over_periodstest_invest_linked_periodstest_effect_period_weightsB3. Scenarios (
test_scenarios.py, newTestScenarios)test_scenario_weights_affect_objectivetest_scenario_independent_sizestest_scenario_independent_flow_ratesB4. Clustering (
test_clustering.py, newTestClustering)These tests are structural/approximate (clustering is heuristic). Require
tsam(pytest.importorskip).test_clustering_basic_objectivetest_storage_cluster_mode_cyclictest_storage_cluster_mode_interclustertest_status_cluster_mode_cyclicSummary Table
Total: 25 new tests (×3 optimize modes = 75 test runs)
Implementation order
Testing
python -m pytest tests/test_math/ -v --tb=short— all tests should pass across all 3 optimize modesChecklist
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
statisticsaccessor tostats(e.g.,flow_system.stats.plot.balance())weightsproperty toscenario_weightsfrom_old_dataset()method for loading legacy datasetscluster_modeparameter to control clustering behaviorbefore_solvecallback hook for custom constraintsinputs,data_only, andthresholdaccessorsBug Fixes
Chores