Feature/properties #557
Feature/properties #557
Conversation
Complete rework of the clustering/aggregation system: - New clustering module with inter-cluster linking and storage modes - Unified cluster/expand API via transform_accessor - Support for multi-period and multi-scenario clustering - Improved cluster weights and statistics calculations - Storage model support for clustered operation (initial=equal, cyclic, etc.) - External tsam support with data aggregation - Cluster IO and serialization 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add new dataset plotting accessor for enhanced visualization: - fxplot accessor for xarray datasets - Improved plotting defaults and styling - Animation frame handling improvements 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add comparison functionality for analyzing multiple FlowSystem solutions: - Compare results across different scenarios - Visualization support for comparison data 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Tests for cluster_reduce and expand functionality - Tests for clustering base and integration - Tests for clustering IO - Tests for single selection (sel/isel) - Updated existing tests to work with new API 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add new clustering notebooks (08c, 08c2, 08d, 08e) - Add cluster architecture design documentation - Add tutorial data helper module for notebooks - Update existing notebooks to use new API - Improve user guide documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Document all breaking changes and new features for the major release. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Improve CI workflow for faster notebook execution in documentation builds. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
| Case | Layout |
|------------------|-------------------------------------------------------------------------|
| Simple (no dims) | Single heatmap |
| Periods only | Vertical stack (1 column) |
| Scenarios only | Horizontal layout |
| Both | Grid (periods as rows, scenarios as columns via facet_cols=n_scenarios) |
| File | Issue | Fix |
|-------------------------------------------------|-----------------------------------------------------------|-----------------------------------------------------|
| docs/notebooks/03-investment-optimization.ipynb | Division by zero when solar_size = 0 | Added guard: if solar_size > 0 else float('nan') |
| flixopt/clustering/base.py:181-235 | ClusterStructure.plot() crashes for multi-period/scenario | Added NotImplementedError with helpful message |
| flixopt/components.py:1256-1265 | Obsolete linopy LP writer bug workaround | Removed + 0.0 workaround (fixed in linopy >= 0.5.1) |
| flixopt/dataset_plot_accessor.py:742-780 | to_duration_curve crashes on variables without time dim | Added guard to skip non-time variables |
| flixopt/features.py:234-236 | Critical: Startup count ignores cluster weighting | Now multiplies by cluster_weight before summing |
| flixopt/structure.py:268-281 | scenario_weights docstring misleading | Updated docstring to accurately describe behavior |
Nitpick Fixes
| File | Fix |
|----------------------------------------------------|-------------------------------------------------------------------------------------------|
| docs/notebooks/data/generate_example_systems.py | Fixed type hint pd.DataFrame → pd.Series for _elec_prices, added timezone guard |
| flixopt/statistics_accessor.py:2103-2132 | Added detailed comment explaining secondary y-axis offset strategy |
| tests/test_cluster_reduce_expand.py | Moved import xarray as xr to top of file |
| docs/notebooks/data/generate_realistic_profiles.py | Clarified warnings.resetwarnings() comment |
| flixopt/transform_accessor.py | Removed unused cluster_coords and time_coords params from _combine_slices_to_dataarray_2d |
| flixopt/comparison.py | Added error handling to _concat_property for FlowSystems lacking optimization data |
1. Added _invalidate() helper method to the Interface base class in structure.py
2. Made Element.label immutable - attempting to set label now raises AttributeError
3. Converted parameters to properties across all Element/Component classes:
- Flow: size, relative_minimum/maximum, fixed_relative_profile, effects_per_flow_hour, status_parameters, flow_hours constraints, load_factor bounds
- Bus: carrier, imbalance_penalty_per_flow_hour
- Component: inputs, outputs, status_parameters, prevent_simultaneous_flows
- Storage: capacity_in_flow_hours, eta_charge/discharge, initial_charge_state, all charging bounds
- LinearConverter: conversion_factors, piecewise_conversion
- Transmission, Source, Sink: all relevant parameters
- Linear converter subclasses: Boiler, CHP, HeatPump, etc. (including efficiency properties)
- InvestParameters, StatusParameters: all parameters
- Effect: unit, description, is_standard, is_objective, period_weights, share_from_*, all bounds
4. Created tests/test_property_guards.py with 19 tests verifying:
- Label immutability (4 tests)
- Modification before linking works (3 tests)
- Modification after linking invalidates FlowSystem (3 tests)
- Nested object modification invalidates (2 tests)
- List/dict properties return copies (2 tests)
- Full workflow: modify → re-optimize (1 test)
- Storage properties invalidate (3 tests)
- HeatPump COP invalidates (1 test)
5. All 1561 tests pass including the new property guard tests
Key Design Decisions
- Labels are immutable (raise AttributeError)
- All other params are mutable but trigger _invalidate() after linking
- List/dict properties return copies to prevent in-place mutation
- Internal code uses backing fields (self._size) to avoid triggering invalidation during transform_data()
Resolve 24 merge conflicts, bringing improvements from main: - tsam clustering with storage inter-cluster linking - fxplot accessor and comparison module - Improved error handling, logging, and docstrings - CI speedups and notebook improvements 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring across nine core modules, systematically replacing public attributes with private backing fields and exposing them through property accessors with invalidation semantics. The changes support a cluster-aware architecture and prepare the codebase for inter-cluster storage linking, documented in a new design guide. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧬 Code graph analysis (1)flixopt/interface.py (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (7)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/notebooks/08a-aggregation.ipynb (1)
111-117: Minor comment inconsistency.The comment on line 113 says "Resample from 15-min to 4h resolution", but based on the print statement on line 68 the system appears to be at hourly resolution. Consider updating the comment for accuracy.
- "# Resample from 15-min to 4h resolution\n", + "# Resample from 1h to 4h resolution\n",flixopt/flow_system.py (1)
2065-2105: Use.item()to safely extract boolean from xarray operationThe code
if np.isclose(norm, 0.0).any():has a truth value ambiguity issue. Sincenormis an xarray.DataArray,np.isclose(norm, 0.0)returns a DataArray, and.any()on a DataArray returns a 0-dimensional DataArray (not a Python bool). Using it directly in an if statement can raise a truth value error.The simplest fix is to convert the result to a Python bool using
.item():- if np.isclose(norm, 0.0).any(): + if np.isclose(norm, 0.0).any().item():This preserves all existing logic including the detailed error diagnostics for multi-dimensional weights.
🤖 Fix all issues with AI agents
In @docs/notebooks/data/generate_example_systems.py:
- Around line 543-547: The comment mismatches the generated timesteps and the
described year: update the comment around the pd.date_range(...) and
_get_weather() usage to clearly state that timesteps are generated for a
non‑leap reference year (2019) to match TMY's 8760 hours and that the
temperature and ghi arrays are from the TMY-like weather returned by
_get_weather(), e.g., clarify that we "map TMY (8760-hour) weather from
_get_weather() onto a 2019 non‑leap hourly index (pd.date_range(...))" so
variables timesteps, temp, and ghi are unambiguous.
In @docs/notebooks/data/generate_realistic_profiles.py:
- Around line 185-235: The PV output scaling in generate_pv_profile is incorrect
because system_efficiency is applied on top of capacity_kw, causing peak power
to be ~0.1275× capacity_kw; remove the hardcoded system_efficiency multiplier
(or add a new parameter performance_ratio with default=1.0) and compute
pv_output as poa['poa_global'] * capacity_kw / 1000 (optionally multiply by
performance_ratio if you want to model losses). Update references to pv_output
and the final clipping so values are in kW and clipped to [0, capacity_kw], and
document the new performance_ratio parameter if added.
In @flixopt/clustering/base.py:
- Around line 331-337: ClusterResult._create_reference_structure currently
mutates self.n_representatives when it's an xr.DataArray; instead create a local
renamed copy and register that in arrays without assigning back to self: if
isinstance(self.n_representatives, xr.DataArray) then compute n_rep_name, make a
renamed copy (e.g., n_representatives_da =
self.n_representatives.rename(n_rep_name)), store arrays[n_rep_name] =
n_representatives_da and set ref['n_representatives'] = f':::{n_rep_name}';
otherwise keep ref['n_representatives'] = int(self.n_representatives). Ensure
you do not reassign to self.n_representatives anywhere in
_create_reference_structure.
- Around line 110-133: The method _create_reference_structure mutates
self.n_clusters by assigning a renamed DataArray back to it; avoid this by
creating a local renamed copy instead of reassigning to self.n_clusters. Inside
the isinstance(self.n_clusters, xr.DataArray) branch, compute n_clusters_name =
self.n_clusters.name or 'n_clusters', set renamed =
self.n_clusters.rename(n_clusters_name) (do not assign back to self.n_clusters),
add arrays[n_clusters_name] = renamed and ref['n_clusters'] =
f':::{n_clusters_name}' so you preserve the original attribute and still
serialize the renamed copy; keep the rest of the method (cluster_order,
cluster_occurrences, timesteps_per_cluster) unchanged.
In @flixopt/clustering/intercluster_helpers.py:
- Around line 49-53: The module-level constant DEFAULT_UNBOUNDED_CAPACITY is set
to 1e6 but the docstring later refers to 1e9; make them consistent by updating
the docstring text that mentions the default upper bound (the paragraph
referencing InvestParameters.maximum_size) to state 1e6 instead of 1e9 so the
documentation matches DEFAULT_UNBOUNDED_CAPACITY.
In @flixopt/transform_accessor.py:
- Around line 1293-1334: The boolean check on the xarray DataArray loss_value
inside _apply_soc_decay is ambiguous; replace the current if not (loss_value >
0).any(): with a concrete scalar check by converting the DataArray to a
numpy/scalar before testing (e.g., use loss_value.values.any() or
bool((loss_value > 0).values.any()) or loss_value.any().item()), so the early
return when self-discharge is zero works correctly and avoids the ValueError
raised by evaluating a DataArray in a boolean context.
In
@tests/deprecated/examples/03_Optimization_modes/example_optimization_modes.py:
- Around line 192-217: The wrapper currently uses a placeholder duration; change
the optimize call to capture its return value (e.g., optimization_result =
clustered_fs.optimize(...)), update the ClusteredResult class signature to
accept the optimization object (def __init__(self, name, fs, optimization):) and
set self.durations = optimization.durations, and then instantiate with
ClusteredResult('Clustered', clustered_fs, optimization_result) so the actual
solve time is preserved.
🧹 Nitpick comments (16)
.github/workflows/docs.yaml (1)
124-144: DRY opportunity: Consider extracting notebook execution to a composite action.The notebook caching and execution logic is duplicated between
buildanddeployjobs. If this pattern will be maintained long-term, consider extracting it into a reusable composite action or a shared workflow to reduce maintenance burden.That said, this duplication is acceptable for now given the workflow's purpose.
docs/design/cluster_architecture.md (1)
37-40: Add language specifier to fenced code block.The code block starting at line 37 is missing a language specifier, which helps with syntax highlighting and accessibility.
📝 Suggested fix
-``` +```text time: (n_clusters × timesteps_per_cluster,) # Flat, e.g., (864,) for 9 clusters × 96 timesteps</details> </blockquote></details> <details> <summary>flixopt/effects.py (1)</summary><blockquote> `240-285`: **Consider whether `unit` and `description` setters should trigger invalidation.** Setting `unit` or `description` typically doesn't affect the mathematical model - these are metadata fields. Calling `_invalidate()` on these setters may cause unnecessary model rebuilds. If these are purely informational (as the docstring suggests for `unit`), consider removing the `_invalidate()` call from their setters: <details> <summary>♻️ Optional: Skip invalidation for metadata-only fields</summary> ```diff @unit.setter def unit(self, value: str) -> None: self._unit = value - self._invalidate() @description.setter def description(self, value: str) -> None: self._description = value - self._invalidate()flixopt/clustering/base.py (1)
1154-1172: Potential inefficiency in create_cluster_structure_from_mapping.The loop at lines 1171-1172 uses a list comprehension inside a loop to count occurrences, resulting in O(n²) complexity. For large cluster counts, this could be slow.
♻️ More efficient occurrence counting
# Count occurrences of each cluster unique_clusters = np.unique(cluster_order) n_clusters = int(unique_clusters.max()) + 1 if len(unique_clusters) > 0 else 0 - occurrences = {} - for c in unique_clusters: - occurrences[int(c)] = sum(1 for x in cluster_order if x == c) + # Use numpy's bincount for O(n) counting + occurrences = dict(enumerate(np.bincount(cluster_order, minlength=n_clusters)))flixopt/statistics_accessor.py (2)
129-170: Consider clarifying the slot calculation logic.The slot calculation on line 166 uses boolean addition, which is compact but potentially unclear:
slots = (facet_col == 'auto') + (animation_frame == 'auto')While functionally correct, this could be more explicit for maintainability.
♻️ More explicit slot calculation
- slots = (facet_col == 'auto') + (animation_frame == 'auto') + # Count available slots for extra dimensions (facet_col and animation_frame) + available_slots = sum([facet_col == 'auto', animation_frame == 'auto']) + slots = available_slotsOr even simpler:
- slots = (facet_col == 'auto') + (animation_frame == 'auto') + slots = sum([facet_col == 'auto', animation_frame == 'auto'])
2093-2136: The axis naming and +100 offset strategy are correct; consider adding validation for trace-to-axis mapping.The implementation correctly leverages plotly's 'yaxis', 'yaxis2', etc. naming scheme for faceted subplots, and the +100 offset is a reasonable collision-avoidance strategy. However, the code assumes that
line_fig.datatraces are ordered to match the sorted primary axes without explicit validation.While plotly.express typically generates faceted traces in consistent order, this mapping relies on undocumented behavior. If plotly's faceting logic changes, traces could be assigned to incorrect secondary axes without detection.
Add a simple assertion after creating the secondary axes:
assert len(line_fig.data) == len(primary_yaxes), \ f"Trace count ({len(line_fig.data)}) != axis count ({len(primary_yaxes)})"This catches mismatches early and makes the coupling to plotly's internal behavior explicit.
flixopt/optimize_accessor.py (1)
88-99: Consider moving imports to module level for consistency.The deprecation logic is correct, but importing
warningsandDEPRECATION_REMOVAL_VERSIONinside the conditional block is inconsistent with the rest of the module's import style. While functional, moving them to the top would improve consistency.♻️ Optional refactor
Move the imports to the module level (after line 17):
from .config import CONFIG +from .config import DEPRECATION_REMOVAL_VERSION from .io import suppress_output +import warningsThen simplify the conditional block:
if normalize_weights is not None: - import warnings - - from .config import DEPRECATION_REMOVAL_VERSION - warnings.warn(docs/notebooks/data/raw/README.md (1)
1-31: LGTM! Clear documentation of bundled data sources.The README provides comprehensive information about the datasets including source, license, and column descriptions. The attribution and licensing information is properly documented, which is important for data provenance.
Optional: Format URLs as markdown links
For better markdown formatting, consider converting bare URLs to links:
-**URL:** https://re.jrc.ec.europa.eu/pvg_tools/en/ +**URL:** [PVGIS Tools](https://re.jrc.ec.europa.eu/pvg_tools/en/)-**URL:** https://data.open-power-system-data.org/time_series/ +**URL:** [Open Power System Data - Time Series](https://data.open-power-system-data.org/time_series/)-**Attribution required:** "Data from Open Power System Data. https://open-power-system-data.org" +**Attribution required:** "Data from Open Power System Data. [https://open-power-system-data.org](https://open-power-system-data.org)"tests/deprecated/examples/03_Optimization_modes/example_optimization_modes.py (1)
178-178: Type annotation doesn't include ClusteredResult (acceptable for deprecated code).The type annotation
list[fx.Optimization | fx.SegmentedOptimization]doesn't includeClusteredResult, but the code appends aClusteredResultinstance (line 217). This is acceptable because:
- This is in the
deprecated/examplesdirectoryget_solutions()uses structural typing (duck typing) and only requires.nameand.flow_systemattributes- Adding a formal type would require a Protocol or base class, which is overkill for deprecated code
Consider adding a comment explaining this if the type checker complains.
docs/notebooks/data/generate_example_systems.py (2)
20-43: Consider documenting the import handling strategy.The try/except import pattern handles multiple execution contexts (direct run, package import, mkdocs-jupyter), which is good. However, this complexity could benefit from a brief comment explaining why both import strategies are needed and when each applies.
📝 Suggested documentation
-# Handle imports in different contexts (direct run, package import, mkdocs-jupyter) +# Handle imports in different contexts: +# 1. Relative import: when run as part of flixopt package (docs build, tests) +# 2. Absolute import: when run directly or in mkdocs-jupyter (sys.path manipulation) try: from .generate_realistic_profiles import (
56-87: Global state in notebook/script context is acceptable; refactor only if this becomes library code.The lazy-loading globals work well for single-threaded notebook execution where data is loaded once but reused across multiple example system generation functions. While
pytest-xdistis available in the project, this file is not used in test suites. If this code is later refactored into reusable library functions or imported in parallel contexts, consider migrating tofunctools.lru_cachefor thread safety.docs/notebooks/data/tutorial_data.py (1)
48-49: Consider using distinct seeds across functions.The seed
42is reused inget_heat_system_data()(line 48),get_investment_data()(line 75), andget_scenarios_data()(line 227). While this ensures reproducibility within each function, the identical seeds mean these functions will produce the same random sequence, which could lead to unintended correlations in the generated noise patterns.If this is intentional for consistency across tutorials, consider adding a brief comment explaining the choice. Otherwise, consider using unique seeds per function.
Also applies to: 75-76, 227-228
flixopt/optimization.py (1)
80-124: Missing deprecation warning fornormalize_weightsparameter.The
normalize_weightsparameter is marked as deprecated in the docstring (line 98) and is now always set toTrue(line 110), but there's no deprecation warning emitted when a caller explicitly passes a value. TheFlowSystem.create_model()method (from relevant snippets) does emit such a warning. Consider adding a warning here for consistency.♻️ Suggested improvement
def _initialize_optimization_common( obj: Any, name: str, flow_system: FlowSystem, folder: pathlib.Path | None = None, normalize_weights: bool | None = None, ) -> None: ... obj.name = name if flow_system.used_in_calculation: logger.warning( f'This FlowSystem is already used in an optimization:\n{flow_system}\n' f'Creating a copy of the FlowSystem for Optimization "{obj.name}".' ) flow_system = flow_system.copy() + if normalize_weights is not None: + warnings.warn( + f'normalize_weights parameter is deprecated and will be removed in {DEPRECATION_REMOVAL_VERSION}. ' + 'Scenario weights are now always normalized when set on FlowSystem.', + DeprecationWarning, + stacklevel=3, + ) + # normalize_weights is deprecated but kept for backwards compatibility obj.normalize_weights = True # Always True nowdocs/notebooks/data/generate_realistic_profiles.py (1)
28-31: Avoid resetting global warnings filters at module importCalling
warnings.resetwarnings()on import can undo project-level warning configuration (e.g. in tests or user scripts). Prefer scoping any suppression to specific calls (viawarnings.catch_warnings) or filtering only the exact warnings you want to silence from dependencies.flixopt/structure.py (1)
438-447: Be more selective about invalidation for cosmetic Element properties
Interface._invalidate()is used byElement.meta_dataandElement.colorsetters to callFlowSystem._invalidate_model()whenever these properties change. That has two side effects:
- It tears down the model and resets
connected_and_transformed, even though meta_data/color don’t affect optimization.- It can run on a locked FlowSystem (solution present), leading to a state where
is_lockedis still True butmodelhas been cleared.Consider one of these options:
- Don’t invalidate on purely cosmetic fields (
color,meta_data), and reserve_invalidate()for modeling‑relevant parameters only.- Or, have
_invalidate()delegate toflow_system.invalidate()so modifications on a locked system raise consistently instead of silently half-resetting internals.This keeps invalidation semantics aligned with the existing
reset()/invalidate()contract while still providing safety for real parameter changes.Also applies to: 1151-1182
flixopt/dataset_plot_accessor.py (1)
970-1018: DataArray heatmap duplicates logic from Dataset heatmap.The heatmap method in DataArrayPlotAccessor duplicates most of the logic from DatasetPlotAccessor.heatmap. Consider refactoring to reduce duplication.
♻️ Consider extracting shared heatmap logic
The core heatmap logic (slot resolution, facet handling, dimension squeezing) is duplicated between
DatasetPlotAccessor.heatmapandDataArrayPlotAccessor.heatmap. Consider extracting a shared helper function:def _build_heatmap_figure( da: xr.DataArray, ds_for_slots: xr.Dataset, *, colors: str | list[str] | None, title: str, facet_col: str | Literal['auto'] | None, animation_frame: str | Literal['auto'] | None, facet_cols: int | None, **imshow_kwargs: Any, ) -> go.Figure: """Shared heatmap building logic for Dataset and DataArray accessors.""" # ... shared implementationThis would allow both accessors to delegate to the same implementation, reducing maintenance burden.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/notebooks/data/raw/electricity_prices_de_2020.csvis excluded by!**/*.csvdocs/notebooks/data/raw/tmy_dresden.csvis excluded by!**/*.csv
📒 Files selected for processing (79)
.github/workflows/docs.yaml.pre-commit-config.yamlCHANGELOG.mdCITATION.cffdocs/design/cluster_architecture.mddocs/notebooks/02-heat-system.ipynbdocs/notebooks/03-investment-optimization.ipynbdocs/notebooks/04-operational-constraints.ipynbdocs/notebooks/05-multi-carrier-system.ipynbdocs/notebooks/06a-time-varying-parameters.ipynbdocs/notebooks/07-scenarios-and-periods.ipynbdocs/notebooks/08a-aggregation.ipynbdocs/notebooks/08b-rolling-horizon.ipynbdocs/notebooks/08c-clustering.ipynbdocs/notebooks/08c2-clustering-storage-modes.ipynbdocs/notebooks/08d-clustering-multiperiod.ipynbdocs/notebooks/08e-clustering-internals.ipynbdocs/notebooks/09-plotting-and-data-access.ipynbdocs/notebooks/data/__init__.pydocs/notebooks/data/generate_example_systems.pydocs/notebooks/data/generate_realistic_profiles.pydocs/notebooks/data/raw/README.mddocs/notebooks/data/tutorial_data.pydocs/notebooks/fxplot_accessor_demo.ipynbdocs/user-guide/optimization/clustering.mddocs/user-guide/optimization/index.mddocs/user-guide/recipes/plotting-custom-data.mddocs/user-guide/results-plotting.mddocs/user-guide/results/index.mdflixopt/__init__.pyflixopt/clustering.pyflixopt/clustering/__init__.pyflixopt/clustering/base.pyflixopt/clustering/intercluster_helpers.pyflixopt/comparison.pyflixopt/components.pyflixopt/config.pyflixopt/core.pyflixopt/dataset_plot_accessor.pyflixopt/effects.pyflixopt/elements.pyflixopt/features.pyflixopt/flow_system.pyflixopt/interface.pyflixopt/io.pyflixopt/linear_converters.pyflixopt/optimization.pyflixopt/optimize_accessor.pyflixopt/plot_result.pyflixopt/results.pyflixopt/statistics_accessor.pyflixopt/structure.pyflixopt/transform_accessor.pymkdocs.ymlpyproject.tomltests/deprecated/examples/03_Optimization_modes/example_optimization_modes.pytests/deprecated/test_bus.pytests/deprecated/test_effect.pytests/deprecated/test_flow.pytests/deprecated/test_flow_system_resample.pytests/deprecated/test_integration.pytests/deprecated/test_linear_converter.pytests/deprecated/test_scenarios.pytests/deprecated/test_storage.pytests/test_bus.pytests/test_cluster_reduce_expand.pytests/test_clustering/__init__.pytests/test_clustering/test_base.pytests/test_clustering/test_integration.pytests/test_clustering_io.pytests/test_effect.pytests/test_flow.pytests/test_flow_system_resample.pytests/test_io_conversion.pytests/test_linear_converter.pytests/test_property_guards.pytests/test_scenarios.pytests/test_sel_isel_single_selection.pytests/test_storage.py
💤 Files with no reviewable changes (1)
- flixopt/clustering.py
🧰 Additional context used
🧬 Code graph analysis (21)
flixopt/optimize_accessor.py (1)
flixopt/flow_system.py (2)
FlowSystem(55-2512)build_model(1355-1391)
flixopt/clustering/__init__.py (1)
flixopt/clustering/base.py (4)
Clustering(980-1135)ClusterResult(248-544)ClusterStructure(35-244)create_cluster_structure_from_mapping(1138-1185)
tests/test_bus.py (1)
flixopt/structure.py (2)
timestep_duration(220-222)timestep_duration(1793-1794)
tests/deprecated/test_flow_system_resample.py (1)
flixopt/structure.py (2)
timestep_duration(220-222)timestep_duration(1793-1794)
flixopt/clustering/intercluster_helpers.py (1)
flixopt/interface.py (1)
InvestParameters(956-1385)
tests/deprecated/test_integration.py (4)
tests/conftest.py (1)
assert_almost_equal_numeric(696-725)tests/deprecated/conftest.py (1)
assert_almost_equal_numeric(695-724)flixopt/flow_system.py (2)
solution(1449-1466)solution(1469-1472)flixopt/structure.py (2)
solution(169-217)solution(1196-1210)
flixopt/core.py (1)
flixopt/flow_system.py (1)
coords(2003-2021)
tests/deprecated/test_storage.py (1)
flixopt/structure.py (2)
timestep_duration(220-222)timestep_duration(1793-1794)
tests/deprecated/test_bus.py (2)
flixopt/structure.py (3)
variables(1761-1767)timestep_duration(220-222)timestep_duration(1793-1794)flixopt/results.py (2)
variables(335-339)variables(1260-1268)
flixopt/features.py (2)
flixopt/flow_system.py (6)
temporal_weight(1992-2000)temporal_dims(1982-1989)sum_temporal(2139-2156)dims(1938-1958)weights(2117-2137)coords(2003-2021)flixopt/structure.py (11)
temporal_weight(255-257)temporal_dims(247-252)sum_temporal(259-265)dims(229-231)weights(239-244)get(1700-1705)get(1865-1867)timestep_duration(220-222)timestep_duration(1793-1794)get_coords(294-329)get_coords(1707-1712)
tests/deprecated/test_linear_converter.py (3)
flixopt/structure.py (4)
variables(1761-1767)timestep_duration(220-222)timestep_duration(1793-1794)constraints(1752-1758)tests/conftest.py (1)
assert_conequal(755-771)tests/deprecated/conftest.py (1)
assert_conequal(754-770)
flixopt/linear_converters.py (1)
flixopt/structure.py (1)
_invalidate(438-446)
tests/deprecated/test_effect.py (1)
flixopt/structure.py (2)
timestep_duration(220-222)timestep_duration(1793-1794)
tests/deprecated/test_scenarios.py (2)
flixopt/flow_system.py (2)
scenario_weights(2055-2062)scenario_weights(2065-2104)flixopt/structure.py (4)
scenario_weights(268-282)flow_system(449-467)values(1571-1576)values(1858-1859)
docs/notebooks/data/generate_realistic_profiles.py (2)
flixopt/structure.py (2)
values(1571-1576)values(1858-1859)flixopt/transform_accessor.py (1)
resample(165-222)
flixopt/effects.py (2)
flixopt/structure.py (5)
_invalidate(438-446)_fit_coords(469-482)prefix(391-393)_fit_effect_coords(484-502)dims(229-231)flixopt/flow_system.py (1)
dims(1938-1958)
flixopt/interface.py (2)
flixopt/structure.py (7)
_invalidate(438-446)prefix(391-393)_fit_effect_coords(484-502)dims(229-231)transform_data(375-388)_fit_coords(469-482)flow_system(449-467)flixopt/effects.py (1)
transform_data(402-445)
flixopt/structure.py (2)
flixopt/flow_system.py (14)
FlowSystem(55-2512)solution(1449-1466)solution(1469-1472)coords(2003-2021)indexes(1961-1979)dims(1938-1958)weights(2117-2137)temporal_dims(1982-1989)temporal_weight(1992-2000)sum_temporal(2139-2156)scenario_weights(2055-2062)scenario_weights(2065-2104)_unit_weight(2106-2114)_invalidate_model(1482-1503)flixopt/carrier.py (1)
label(107-109)
flixopt/optimization.py (1)
flixopt/flow_system.py (2)
FlowSystem(55-2512)create_model(1333-1353)
flixopt/__init__.py (1)
flixopt/comparison.py (1)
Comparison(27-191)
flixopt/dataset_plot_accessor.py (2)
flixopt/color_processing.py (1)
process_colors(165-233)flixopt/config.py (2)
CONFIG(184-912)Plotting(550-592)
🪛 LanguageTool
docs/design/cluster_architecture.md
[grammar] ~512-~512: Use a hyphen to join words.
Context: ...Goal: Produce true (cluster, time) shaped data. Tasks: 1. Update `clus...
(QB_NEW_EN_HYPHEN)
docs/user-guide/results/index.md
[style] ~333-~333: Using many exclamation marks might seem excessive (in this case: 9 exclamation marks for a text that’s 4730 characters long)
Context: ...Works! cluster_boundary is ignored ``` !!! note "Component Differences" System...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
docs/design/cluster_architecture.md
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
CHANGELOG.md
74-74: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
121-121: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
149-149: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
189-189: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
docs/notebooks/data/raw/README.md
10-10: Bare URL used
(MD034, no-bare-urls)
26-26: Bare URL used
(MD034, no-bare-urls)
28-28: Bare URL used
(MD034, no-bare-urls)
| def create_district_heating_system() -> fx.FlowSystem: | ||
| """Create a district heating system with BDEW profiles. | ||
| Uses realistic German data: | ||
| - One month (January 2020), hourly resolution | ||
| - BDEW industrial heat profile | ||
| - BDEW commercial electricity profile | ||
| - OPSD electricity prices | ||
| - Seasonal gas prices | ||
| - CHP, boiler, storage, and grid connections | ||
| - Investment optimization for sizing | ||
| Used by: 08a-aggregation, 08c-clustering, 08e-clustering-internals notebooks | ||
| """ | ||
| # One month, hourly | ||
| timesteps = pd.date_range('2020-01-01', '2020-01-31 23:00:00', freq='h') | ||
| temp = _get_weather()['temperature_C'].reindex(timesteps, method='ffill').values | ||
|
|
||
| # BDEW profiles (MW scale for district heating) | ||
| thermal_gen = ThermalLoadGenerator() | ||
| heat_demand = thermal_gen.generate(timesteps, temp, 'industrial', annual_demand_kwh=15_000_000) / 1000 # MW | ||
|
|
||
| elec_gen = ElectricityLoadGenerator() | ||
| electricity_demand = elec_gen.generate(timesteps, 'commercial', annual_demand_kwh=5_000_000) / 1000 # MW | ||
|
|
||
| # Prices | ||
| electricity_price = _get_elec_prices().reindex(timesteps, method='ffill').values # EUR/MWh | ||
| gas_gen = GasPriceGenerator() | ||
| gas_price = gas_gen.generate(timesteps) # EUR/MWh | ||
|
|
||
| fs = fx.FlowSystem(timesteps) | ||
| fs.add_elements( | ||
| # Buses | ||
| fx.Bus('Electricity'), | ||
| fx.Bus('Heat'), | ||
| fx.Bus('Gas'), | ||
| fx.Bus('Coal'), | ||
| # Effects | ||
| fx.Effect('costs', '€', 'Total Costs', is_standard=True, is_objective=True), | ||
| fx.Effect('CO2', 'kg', 'CO2 Emissions'), | ||
| # CHP unit with investment | ||
| fx.linear_converters.CHP( | ||
| 'CHP', | ||
| thermal_efficiency=0.58, | ||
| electrical_efficiency=0.22, | ||
| electrical_flow=fx.Flow('P_el', bus='Electricity', size=200), | ||
| thermal_flow=fx.Flow( | ||
| 'Q_th', | ||
| bus='Heat', | ||
| size=fx.InvestParameters( | ||
| minimum_size=100, | ||
| maximum_size=300, | ||
| effects_of_investment_per_size={'costs': 10}, | ||
| ), | ||
| relative_minimum=0.3, | ||
| status_parameters=fx.StatusParameters(), | ||
| ), | ||
| fuel_flow=fx.Flow('Q_fu', bus='Coal'), | ||
| ), | ||
| # Gas Boiler with investment | ||
| fx.linear_converters.Boiler( | ||
| 'Boiler', | ||
| thermal_efficiency=0.85, | ||
| thermal_flow=fx.Flow( | ||
| 'Q_th', | ||
| bus='Heat', | ||
| size=fx.InvestParameters( | ||
| minimum_size=0, | ||
| maximum_size=150, | ||
| effects_of_investment_per_size={'costs': 5}, | ||
| ), | ||
| relative_minimum=0.1, | ||
| status_parameters=fx.StatusParameters(), | ||
| ), | ||
| fuel_flow=fx.Flow('Q_fu', bus='Gas'), | ||
| ), | ||
| # Thermal Storage with investment | ||
| fx.Storage( | ||
| 'Storage', | ||
| capacity_in_flow_hours=fx.InvestParameters( | ||
| minimum_size=0, | ||
| maximum_size=1000, | ||
| effects_of_investment_per_size={'costs': 0.5}, | ||
| ), | ||
| initial_charge_state=0, | ||
| eta_charge=1, | ||
| eta_discharge=1, | ||
| relative_loss_per_hour=0.001, | ||
| charging=fx.Flow('Charge', size=137, bus='Heat'), | ||
| discharging=fx.Flow('Discharge', size=158, bus='Heat'), | ||
| ), | ||
| # Fuel sources | ||
| fx.Source( | ||
| 'GasGrid', | ||
| outputs=[fx.Flow('Q_Gas', bus='Gas', size=1000, effects_per_flow_hour={'costs': gas_price, 'CO2': 0.3})], | ||
| ), | ||
| fx.Source( | ||
| 'CoalSupply', | ||
| outputs=[fx.Flow('Q_Coal', bus='Coal', size=1000, effects_per_flow_hour={'costs': 4.6, 'CO2': 0.3})], | ||
| ), | ||
| # Electricity grid | ||
| fx.Source( | ||
| 'GridBuy', | ||
| outputs=[ | ||
| fx.Flow( | ||
| 'P_el', | ||
| bus='Electricity', | ||
| size=1000, | ||
| effects_per_flow_hour={'costs': electricity_price + 0.5, 'CO2': 0.3}, | ||
| ) | ||
| ], | ||
| ), | ||
| fx.Sink( | ||
| 'GridSell', | ||
| inputs=[fx.Flow('P_el', bus='Electricity', size=1000, effects_per_flow_hour=-(electricity_price - 0.5))], | ||
| ), | ||
| # Demands | ||
| fx.Sink('HeatDemand', inputs=[fx.Flow('Q_th', bus='Heat', size=1, fixed_relative_profile=heat_demand)]), | ||
| fx.Sink( | ||
| 'ElecDemand', inputs=[fx.Flow('P_el', bus='Electricity', size=1, fixed_relative_profile=electricity_demand)] | ||
| ), | ||
| ) | ||
| return fs |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the file and check its structure
fd "generate_example_systems" --type f
# Search for investment parameters across the codebase
rg "effects_of_investment_per_size" --type=py -A2 -B2Repository: flixOpt/flixopt
Length of output: 33452
🏁 Script executed:
#!/bin/bash
# Get the district heating system context with more lines
sed -n '293,415p' docs/notebooks/data/generate_example_systems.py | head -70
# Also check if there are comments or documentation for these cost values
sed -n '340,385p' docs/notebooks/data/generate_example_systems.pyRepository: flixOpt/flixopt
Length of output: 4359
🏁 Script executed:
#!/bin/bash
# Look for other heating systems or comparison examples
grep -n "CHP\|Boiler\|create_.*system" docs/notebooks/data/generate_example_systems.py | head -20
# Check context around lines with high investment costs to see the pattern
grep -B5 -A2 "effects_of_investment_per_size.*15000\|effects_of_investment_per_size.*20000" docs/notebooks/data/generate_example_systems.pyRepository: flixOpt/flixopt
Length of output: 1582
Add documentation or reconsider investment cost values for consistency with other MW-scale systems.
The district heating system's investment parameters (CHP: 10, Boiler: 5, Storage: 0.5 €/unit) lack explanatory comments and are 1500–40000× lower than comparable MW-scale renewable systems in the same file (Solar: 15000 €/MW, Wind: 20000 €/MW, both documented as "annualized"). Either clarify the cost units and assumptions for these values to match the pattern used elsewhere, or verify the values themselves are intentionally lower and document why.
| # Full year, hourly (use non-leap year to match TMY data which has 8760 hours) | ||
| timesteps = pd.date_range('2019-01-01', periods=8760, freq='h') | ||
| # Map to 2020 weather data (TMY has 8760 hours, no Feb 29) | ||
| temp = _get_weather()['temperature_C'].values | ||
| ghi = _get_weather()['ghi_W_m2'].values |
There was a problem hiding this comment.
Clarify comment about weather data year mapping.
The comment at line 546 states "Map to 2020 weather data" but the timesteps are generated for 2019 (line 544). This appears intentional (to match TMY data's 8760 hours), but the comment could be clearer:
📝 Suggested clarification
- # Map to 2020 weather data (TMY has 8760 hours, no Feb 29)
+ # Use 2020 weather data (TMY typical year with 8760 hours, no Feb 29)
+ # Note: timesteps use 2019 to ensure 8760 hours (2020 is a leap year with 8784 hours)
temp = _get_weather()['temperature_C'].values🤖 Prompt for AI Agents
In @docs/notebooks/data/generate_example_systems.py around lines 543 - 547, The
comment mismatches the generated timesteps and the described year: update the
comment around the pd.date_range(...) and _get_weather() usage to clearly state
that timesteps are generated for a non‑leap reference year (2019) to match TMY's
8760 hours and that the temperature and ghi arrays are from the TMY-like weather
returned by _get_weather(), e.g., clarify that we "map TMY (8760-hour) weather
from _get_weather() onto a 2019 non‑leap hourly index (pd.date_range(...))" so
variables timesteps, temp, and ghi are unambiguous.
| def generate_pv_profile( | ||
| self, | ||
| timesteps: pd.DatetimeIndex, | ||
| weather: pd.DataFrame, | ||
| surface_tilt: float = 35, | ||
| surface_azimuth: float = 180, # South-facing | ||
| capacity_kw: float = 1.0, | ||
| ) -> np.ndarray: | ||
| """Generate PV power output profile. | ||
| Parameters | ||
| ---------- | ||
| timesteps | ||
| Time index for the profile | ||
| weather | ||
| Weather data with 'ghi_W_m2', 'dni_W_m2', 'dhi_W_m2', 'temperature_C' | ||
| surface_tilt | ||
| Panel tilt angle in degrees (0=horizontal, 90=vertical) | ||
| surface_azimuth | ||
| Panel azimuth in degrees (180=south, 90=east, 270=west) | ||
| capacity_kw | ||
| Installed PV capacity in kW | ||
| Returns | ||
| ------- | ||
| np.ndarray | ||
| PV power output in kW | ||
| """ | ||
| # Ensure weather is aligned with timesteps | ||
| weather = weather.reindex(timesteps, method='ffill') | ||
|
|
||
| # Get solar position | ||
| solar_position = self.location.get_solarposition(timesteps) | ||
|
|
||
| # Calculate plane-of-array irradiance | ||
| poa = pvlib.irradiance.get_total_irradiance( | ||
| surface_tilt=surface_tilt, | ||
| surface_azimuth=surface_azimuth, | ||
| solar_zenith=solar_position['apparent_zenith'], | ||
| solar_azimuth=solar_position['azimuth'], | ||
| dni=weather['dni_W_m2'], | ||
| ghi=weather['ghi_W_m2'], | ||
| dhi=weather['dhi_W_m2'], | ||
| ) | ||
|
|
||
| # Simple efficiency model: ~15% module efficiency, ~85% system efficiency | ||
| system_efficiency = 0.15 * 0.85 | ||
| pv_output = poa['poa_global'] * system_efficiency * capacity_kw / 1000 | ||
|
|
||
| return np.clip(pv_output.fillna(0).values, 0, capacity_kw) | ||
|
|
There was a problem hiding this comment.
PV capacity scaling is off by a large factor
pv_output = poa['poa_global'] * system_efficiency * capacity_kw / 1000 gives a peak output of only ~0.1275 × capacity_kw at 1000 W/m², even before clipping. For capacity_kw=1, the maximum is ~0.13 kW, not 1 kW.
If capacity_kw is intended as installed DC capacity (kWp), the usual relation is:
[
P_\text{out} [\text{kW}] \approx \text{POA}[W/m^2] \times \frac{\text{capacity_kw}}{1000}
]
i.e. capacity already encodes module+system efficiency. The extra system_efficiency factor should either be removed or folded into how capacity_kw is defined.
A minimal fix that treats capacity_kw as kWp:
- system_efficiency = 0.15 * 0.85
- pv_output = poa['poa_global'] * system_efficiency * capacity_kw / 1000
+ # Treat `capacity_kw` as installed DC capacity (kWp).
+ # Scale POA irradiance (W/m²) to kW using 1000 W/m² reference.
+ pv_output = poa['poa_global'] * capacity_kw / 1000Optionally add a separate performance-ratio parameter if you want to model additional system losses explicitly.
📝 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.
| def generate_pv_profile( | |
| self, | |
| timesteps: pd.DatetimeIndex, | |
| weather: pd.DataFrame, | |
| surface_tilt: float = 35, | |
| surface_azimuth: float = 180, # South-facing | |
| capacity_kw: float = 1.0, | |
| ) -> np.ndarray: | |
| """Generate PV power output profile. | |
| Parameters | |
| ---------- | |
| timesteps | |
| Time index for the profile | |
| weather | |
| Weather data with 'ghi_W_m2', 'dni_W_m2', 'dhi_W_m2', 'temperature_C' | |
| surface_tilt | |
| Panel tilt angle in degrees (0=horizontal, 90=vertical) | |
| surface_azimuth | |
| Panel azimuth in degrees (180=south, 90=east, 270=west) | |
| capacity_kw | |
| Installed PV capacity in kW | |
| Returns | |
| ------- | |
| np.ndarray | |
| PV power output in kW | |
| """ | |
| # Ensure weather is aligned with timesteps | |
| weather = weather.reindex(timesteps, method='ffill') | |
| # Get solar position | |
| solar_position = self.location.get_solarposition(timesteps) | |
| # Calculate plane-of-array irradiance | |
| poa = pvlib.irradiance.get_total_irradiance( | |
| surface_tilt=surface_tilt, | |
| surface_azimuth=surface_azimuth, | |
| solar_zenith=solar_position['apparent_zenith'], | |
| solar_azimuth=solar_position['azimuth'], | |
| dni=weather['dni_W_m2'], | |
| ghi=weather['ghi_W_m2'], | |
| dhi=weather['dhi_W_m2'], | |
| ) | |
| # Simple efficiency model: ~15% module efficiency, ~85% system efficiency | |
| system_efficiency = 0.15 * 0.85 | |
| pv_output = poa['poa_global'] * system_efficiency * capacity_kw / 1000 | |
| return np.clip(pv_output.fillna(0).values, 0, capacity_kw) | |
| def generate_pv_profile( | |
| self, | |
| timesteps: pd.DatetimeIndex, | |
| weather: pd.DataFrame, | |
| surface_tilt: float = 35, | |
| surface_azimuth: float = 180, # South-facing | |
| capacity_kw: float = 1.0, | |
| ) -> np.ndarray: | |
| """Generate PV power output profile. | |
| Parameters | |
| ---------- | |
| timesteps | |
| Time index for the profile | |
| weather | |
| Weather data with 'ghi_W_m2', 'dni_W_m2', 'dhi_W_m2', 'temperature_C' | |
| surface_tilt | |
| Panel tilt angle in degrees (0=horizontal, 90=vertical) | |
| surface_azimuth | |
| Panel azimuth in degrees (180=south, 90=east, 270=west) | |
| capacity_kw | |
| Installed PV capacity in kW | |
| Returns | |
| ------- | |
| np.ndarray | |
| PV power output in kW | |
| """ | |
| # Ensure weather is aligned with timesteps | |
| weather = weather.reindex(timesteps, method='ffill') | |
| # Get solar position | |
| solar_position = self.location.get_solarposition(timesteps) | |
| # Calculate plane-of-array irradiance | |
| poa = pvlib.irradiance.get_total_irradiance( | |
| surface_tilt=surface_tilt, | |
| surface_azimuth=surface_azimuth, | |
| solar_zenith=solar_position['apparent_zenith'], | |
| solar_azimuth=solar_position['azimuth'], | |
| dni=weather['dni_W_m2'], | |
| ghi=weather['ghi_W_m2'], | |
| dhi=weather['dhi_W_m2'], | |
| ) | |
| # Treat `capacity_kw` as installed DC capacity (kWp). | |
| # Scale POA irradiance (W/m²) to kW using 1000 W/m² reference. | |
| pv_output = poa['poa_global'] * capacity_kw / 1000 | |
| return np.clip(pv_output.fillna(0).values, 0, capacity_kw) |
🤖 Prompt for AI Agents
In @docs/notebooks/data/generate_realistic_profiles.py around lines 185 - 235,
The PV output scaling in generate_pv_profile is incorrect because
system_efficiency is applied on top of capacity_kw, causing peak power to be
~0.1275× capacity_kw; remove the hardcoded system_efficiency multiplier (or add
a new parameter performance_ratio with default=1.0) and compute pv_output as
poa['poa_global'] * capacity_kw / 1000 (optionally multiply by performance_ratio
if you want to model losses). Update references to pv_output and the final
clipping so values are in kW and clipped to [0, capacity_kw], and document the
new performance_ratio parameter if added.
| def _create_reference_structure(self) -> tuple[dict, dict[str, xr.DataArray]]: | ||
| """Create reference structure for serialization.""" | ||
| ref = {'__class__': self.__class__.__name__} | ||
| arrays = {} | ||
|
|
||
| # Store DataArrays with references | ||
| arrays[str(self.cluster_order.name)] = self.cluster_order | ||
| ref['cluster_order'] = f':::{self.cluster_order.name}' | ||
|
|
||
| arrays[str(self.cluster_occurrences.name)] = self.cluster_occurrences | ||
| ref['cluster_occurrences'] = f':::{self.cluster_occurrences.name}' | ||
|
|
||
| # Store scalar values | ||
| if isinstance(self.n_clusters, xr.DataArray): | ||
| n_clusters_name = self.n_clusters.name or 'n_clusters' | ||
| self.n_clusters = self.n_clusters.rename(n_clusters_name) | ||
| arrays[n_clusters_name] = self.n_clusters | ||
| ref['n_clusters'] = f':::{n_clusters_name}' | ||
| else: | ||
| ref['n_clusters'] = int(self.n_clusters) | ||
|
|
||
| ref['timesteps_per_cluster'] = self.timesteps_per_cluster | ||
|
|
||
| return ref, arrays |
There was a problem hiding this comment.
Potential mutation of self.n_clusters during serialization.
Line 125 mutates self.n_clusters by renaming it. Since this is a dataclass with n_clusters: int | xr.DataArray, mutating it during _create_reference_structure could have side effects if the method is called multiple times.
🐛 Suggested fix: Avoid mutating self
if isinstance(self.n_clusters, xr.DataArray):
n_clusters_name = self.n_clusters.name or 'n_clusters'
- self.n_clusters = self.n_clusters.rename(n_clusters_name)
- arrays[n_clusters_name] = self.n_clusters
+ n_clusters_da = self.n_clusters.rename(n_clusters_name)
+ arrays[n_clusters_name] = n_clusters_da
ref['n_clusters'] = f':::{n_clusters_name}'🤖 Prompt for AI Agents
In @flixopt/clustering/base.py around lines 110 - 133, The method
_create_reference_structure mutates self.n_clusters by assigning a renamed
DataArray back to it; avoid this by creating a local renamed copy instead of
reassigning to self.n_clusters. Inside the isinstance(self.n_clusters,
xr.DataArray) branch, compute n_clusters_name = self.n_clusters.name or
'n_clusters', set renamed = self.n_clusters.rename(n_clusters_name) (do not
assign back to self.n_clusters), add arrays[n_clusters_name] = renamed and
ref['n_clusters'] = f':::{n_clusters_name}' so you preserve the original
attribute and still serialize the renamed copy; keep the rest of the method
(cluster_order, cluster_occurrences, timesteps_per_cluster) unchanged.
| if isinstance(self.n_representatives, xr.DataArray): | ||
| n_rep_name = self.n_representatives.name or 'n_representatives' | ||
| self.n_representatives = self.n_representatives.rename(n_rep_name) | ||
| arrays[n_rep_name] = self.n_representatives | ||
| ref['n_representatives'] = f':::{n_rep_name}' | ||
| else: | ||
| ref['n_representatives'] = int(self.n_representatives) |
There was a problem hiding this comment.
Same mutation issue in ClusterResult._create_reference_structure.
Similar to ClusterStructure, line 333 mutates self.n_representatives during serialization.
🐛 Suggested fix
if isinstance(self.n_representatives, xr.DataArray):
n_rep_name = self.n_representatives.name or 'n_representatives'
- self.n_representatives = self.n_representatives.rename(n_rep_name)
- arrays[n_rep_name] = self.n_representatives
+ n_rep_da = self.n_representatives.rename(n_rep_name)
+ arrays[n_rep_name] = n_rep_da
ref['n_representatives'] = f':::{n_rep_name}'🤖 Prompt for AI Agents
In @flixopt/clustering/base.py around lines 331 - 337,
ClusterResult._create_reference_structure currently mutates
self.n_representatives when it's an xr.DataArray; instead create a local renamed
copy and register that in arrays without assigning back to self: if
isinstance(self.n_representatives, xr.DataArray) then compute n_rep_name, make a
renamed copy (e.g., n_representatives_da =
self.n_representatives.rename(n_rep_name)), store arrays[n_rep_name] =
n_representatives_da and set ref['n_representatives'] = f':::{n_rep_name}';
otherwise keep ref['n_representatives'] = int(self.n_representatives). Ensure
you do not reassign to self.n_representatives anywhere in
_create_reference_structure.
| # Default upper bound for unbounded storage capacity. | ||
| # Used when no explicit capacity or InvestParameters.maximum_size is provided. | ||
| # Set to 1e6 to avoid numerical issues with very large bounds while still | ||
| # being effectively unbounded for most practical applications. | ||
| DEFAULT_UNBOUNDED_CAPACITY = 1e6 |
There was a problem hiding this comment.
Documentation inconsistency: default bound value.
The docstring on line 87 mentions 1e9 as the default upper bound, but the actual constant DEFAULT_UNBOUNDED_CAPACITY on line 53 is set to 1e6.
📝 Suggested fix
- - **None/Unbounded**: Upper bound is set to a large value (1e9).
+ - **None/Unbounded**: Upper bound is set to a large value (1e6).Also applies to: 87-88
🤖 Prompt for AI Agents
In @flixopt/clustering/intercluster_helpers.py around lines 49 - 53, The
module-level constant DEFAULT_UNBOUNDED_CAPACITY is set to 1e6 but the docstring
later refers to 1e9; make them consistent by updating the docstring text that
mentions the default upper bound (the paragraph referencing
InvestParameters.maximum_size) to state 1e6 instead of 1e9 so the documentation
matches DEFAULT_UNBOUNDED_CAPACITY.
| self, | ||
| soc_boundary_per_timestep: xr.DataArray, | ||
| storage_name: str, | ||
| cluster_structure, | ||
| original_timesteps_extra: pd.DatetimeIndex, | ||
| original_cluster_indices: np.ndarray, | ||
| timesteps_per_cluster: int, | ||
| ) -> xr.DataArray: | ||
| """Apply self-discharge decay to SOC_boundary values. | ||
| Args: | ||
| soc_boundary_per_timestep: SOC boundary values mapped to each timestep. | ||
| storage_name: Name of the storage component. | ||
| cluster_structure: ClusterStructure with cluster order info. | ||
| original_timesteps_extra: Original timesteps including final extra timestep. | ||
| original_cluster_indices: Mapping of timesteps to original cluster indices. | ||
| timesteps_per_cluster: Number of timesteps per cluster. | ||
| Returns: | ||
| SOC boundary values with decay applied. | ||
| """ | ||
| storage = self._fs.storages.get(storage_name) | ||
| if storage is None: | ||
| return soc_boundary_per_timestep | ||
|
|
||
| n_timesteps = len(original_timesteps_extra) | ||
|
|
||
| # Time within period for each timestep (0, 1, 2, ..., T-1, 0, 1, ...) | ||
| time_within_period = np.arange(n_timesteps) % timesteps_per_cluster | ||
| time_within_period[-1] = timesteps_per_cluster # Extra timestep gets full decay | ||
| time_within_period_da = xr.DataArray( | ||
| time_within_period, dims=['time'], coords={'time': original_timesteps_extra} | ||
| ) | ||
|
|
||
| # Decay factor: (1 - loss)^t | ||
| loss_value = storage.relative_loss_per_hour.mean('time') | ||
| if not (loss_value > 0).any(): | ||
| return soc_boundary_per_timestep | ||
|
|
||
| decay_da = (1 - loss_value) ** time_within_period_da | ||
|
|
||
| # Handle cluster dimension if present |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the specific method to see the exact code
sed -n '1293,1334p' flixopt/transform_accessor.pyRepository: flixOpt/flixopt
Length of output: 1815
🏁 Script executed:
# Also check the context around this method to understand loss_value's type
sed -n '1280,1340p' flixopt/transform_accessor.pyRepository: flixOpt/flixopt
Length of output: 2769
🏁 Script executed:
# Let's verify the structure of storage.relative_loss_per_hour to understand its type
rg "relative_loss_per_hour" -A 2 -B 2Repository: flixOpt/flixopt
Length of output: 23171
🏁 Script executed:
# Check what type storage objects have and where they're defined
rg "class.*[Ss]torage" -A 5Repository: flixOpt/flixopt
Length of output: 8215
🏁 Script executed:
python3 << 'EOF'
import xarray as xr
import numpy as np
# Simulate what happens in the code
time_coords = ['t1', 't2', 't3']
loss_data = np.array([0.01, 0.01, 0.01]) # Example loss values
loss_da = xr.DataArray(loss_data, dims=['time'], coords={'time': time_coords})
# What the code does
loss_value = loss_da.mean('time')
print(f"loss_value type: {type(loss_value)}")
print(f"loss_value: {loss_value}")
# Try the problematic operation
try:
result = (loss_value > 0).any()
print(f"result of (loss_value > 0).any(): {result}")
print(f"result type: {type(result)}")
# Now try using it in an if statement
if not result:
print("In if statement: result is falsy")
else:
print("In if statement: result is truthy")
except ValueError as e:
print(f"Error during comparison: {e}")
# Test with zero loss (the case mentioned in review)
loss_da_zero = xr.DataArray([0.0, 0.0, 0.0], dims=['time'], coords={'time': time_coords})
loss_value_zero = loss_da_zero.mean('time')
try:
result_zero = (loss_value_zero > 0).any()
print(f"\nZero loss case - result: {result_zero}, type: {type(result_zero)}")
if not result_zero:
print("Zero loss: would return early (correct behavior)")
except ValueError as e:
print(f"Error with zero loss: {e}")
EOFRepository: flixOpt/flixopt
Length of output: 177
🏁 Script executed:
# Check if there are tests that exercise _apply_soc_decay
rg "_apply_soc_decay|expand_solution" tests/ -lRepository: flixOpt/flixopt
Length of output: 122
🏁 Script executed:
# Check the exact line with the issue to verify the context
sed -n '1320,1325p' flixopt/transform_accessor.pyRepository: flixOpt/flixopt
Length of output: 446
🏁 Script executed:
# Check if the test suite actually calls _apply_soc_decay via expand_solution
rg "expand_solution" tests/test_cluster_reduce_expand.py -B 2 -A 5Repository: flixOpt/flixopt
Length of output: 7937
🏁 Script executed:
# Look at one of the tests more carefully to see if it exercises soc_decay
sed -n '1,100p' tests/test_cluster_reduce_expand.py | head -60Repository: flixOpt/flixopt
Length of output: 2056
🏁 Script executed:
# Check if there's a test with relative_loss_per_hour that calls expand
rg "relative_loss_per_hour.*0\." tests/test_cluster_reduce_expand.py -B 5 -A 10Repository: flixOpt/flixopt
Length of output: 1781
🏁 Script executed:
# Find where expand_solution calls _apply_soc_decay
rg "def expand_solution" flixopt/transform_accessor.py -A 100 | head -120Repository: flixOpt/flixopt
Length of output: 4958
🏁 Script executed:
# Check if _apply_soc_decay is actually called from expand_solution
sed -n '1400,1500p' flixopt/transform_accessor.pyRepository: flixOpt/flixopt
Length of output: 4636
🏁 Script executed:
# Look for where _apply_soc_decay is invoked to see the code flow
rg "_apply_soc_decay" flixopt/transform_accessor.py -B 3 -A 3Repository: flixOpt/flixopt
Length of output: 601
🏁 Script executed:
# Let's look at the actual test that exercises _apply_soc_decay with self-discharge
sed -n '/test_expand_solution_soc_decay/,/^ def /p' tests/test_cluster_reduce_expand.py | head -50Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
# Check if there's a test with loss_rate = 0.0 (the zero loss case)
rg "relative_loss_per_hour.*=.*0" tests/test_cluster_reduce_expand.py -B 2 -A 8Repository: flixOpt/flixopt
Length of output: 954
🏁 Script executed:
# Look at the actual behavior when xarray comparison result is used in if statement
# by checking xarray documentation or source to understand the error
python3 << 'EOF'
# Simulate the xarray behavior without importing xarray
# When you call .any() on a DataArray comparison result, it returns a DataArray
# Using DataArray in boolean context raises ValueError
# Here's what would happen:
import numpy as np
class FakeDataArray:
def __init__(self, value):
self.value = value
def __bool__(self):
raise ValueError("The truth value of a DataArray is ambiguous. Use a.any() or a.all().")
def __nonzero__(self):
raise ValueError("The truth value of a DataArray is ambiguous. Use a.any() or a.all().")
# This simulates what xarray does
loss_da = FakeDataArray(0.0)
try:
if not loss_da:
print("This won't be reached")
except ValueError as e:
print(f"Error raised as expected: {e}")
# Show what the fix does
print("\nThe fix uses .values to extract numpy array:")
class FixedDataArray:
def __init__(self, value):
self._value = value
@property
def values(self):
return np.array([self._value])
fixed_da = FixedDataArray(0.0)
loss_value_values = fixed_da.values
if not np.any((loss_value_values > 0)):
print("With .values fix: correctly returns early for zero loss")
EOFRepository: flixOpt/flixopt
Length of output: 176
🏁 Script executed:
# Let's find the exact test that exercises SOC decay expansion
sed -n '/def test_expand_solution_soc_decay_with_loss/,/^ def test_/p' tests/test_cluster_reduce_expand.py | head -80Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
# Verify that this test would call _apply_soc_decay by checking the storage config
sed -n '1,50p' tests/test_cluster_reduce_expand.py | grep -A 20 "class.*Storage"Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
# Check if there's actually a test case with cluster_mode='intercluster_cyclic' that exercises _apply_soc_decay
rg "cluster_mode.*intercluster" tests/test_cluster_reduce_expand.py -B 2 -A 15 | head -80Repository: flixOpt/flixopt
Length of output: 4136
🏁 Script executed:
# Let's see the full test that should exercise the code
sed -n '/def test_expand_solution_combines_soc_boundary_with_charge_state/,/^ def test_/p' tests/test_cluster_reduce_expand.py | head -100Repository: flixOpt/flixopt
Length of output: 1128
🏁 Script executed:
# Also check the test with loss (0.01)
sed -n '/def test_expand_solution_soc_decay_with_loss/,/^ def test_/p' tests/test_cluster_reduce_expand.py | head -100Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
# Let's trace through the code - when expand_solution is called, does it call _combine_intercluster_charge_states?
sed -n '1440,1470p' flixopt/transform_accessor.pyRepository: flixOpt/flixopt
Length of output: 1624
🏁 Script executed:
# Check _combine_intercluster_charge_states to confirm it calls _apply_soc_decay
sed -n '1350,1400p' flixopt/transform_accessor.pyRepository: flixOpt/flixopt
Length of output: 2479
🏁 Script executed:
# Final check: look at the exact lines where _apply_soc_decay is called
sed -n '1370,1385p' flixopt/transform_accessor.pyRepository: flixOpt/flixopt
Length of output: 685
🏁 Script executed:
# Search for the actual call to _apply_soc_decay in _combine_intercluster_charge_states
sed -n '1320,1360p' flixopt/transform_accessor.pyRepository: flixOpt/flixopt
Length of output: 2037
🏁 Script executed:
# Get full _combine_intercluster_charge_states method
sed -n '/def _combine_intercluster_charge_states/,/def _apply_soc_decay/p' flixopt/transform_accessor.py | head -100Repository: flixOpt/flixopt
Length of output: 3375
🏁 Script executed:
# Check what happens when you use a DataArray in boolean context
# by looking at what xarray.DataArray.__bool__ does
rg "def __bool__|def __nonzero__" --type py -A 3Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
# Verify the test uses intercluster storage which calls _apply_soc_decay
grep -A 15 "test_expand_solution_combines_soc_boundary_with_charge_state" tests/test_cluster_reduce_expand.py | head -20Repository: flixOpt/flixopt
Length of output: 1032
🏁 Script executed:
# Confirm the storage is created with cluster_mode='intercluster_cyclic'
grep -B 5 "test_expand_solution_combines_soc_boundary_with_charge_state" tests/test_cluster_reduce_expand.py | grep "cluster_mode"Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
# Let me check the xarray version and understand the exact error behavior
cat flixopt/requirements.txt | grep -i xarrayRepository: flixOpt/flixopt
Length of output: 115
🏁 Script executed:
# Also check if there's already handling for this in the codebase
rg "loss_value.*\.values|loss_value.*\.item()" flixopt/ -B 2 -A 2Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
# Check similar patterns in the codebase to see how other boolean checks on DataArrays are handled
rg "if.*\.any\(\)" flixopt/ -B 2 -A 2 | head -50Repository: flixOpt/flixopt
Length of output: 3608
🏁 Script executed:
# Check the default value used by create_system_with_storage in the test
rg "def create_system_with_storage" tests/test_cluster_reduce_expand.py -A 10 | head -15Repository: flixOpt/flixopt
Length of output: 546
Fix boolean check on xarray DataArray in _apply_soc_decay
loss_value is an xarray.DataArray; (loss_value > 0).any() also returns a DataArray. Using it directly in if not (loss_value > 0).any(): raises:
ValueError: The truth value of a DataArray is ambiguous. Use a.any() or a.all().
This triggers whenever _apply_soc_decay is called during expansion of intercluster storage solutions, preventing the intended early return when self-discharge is zero.
Convert to numpy array before calling .any():
loss_value = storage.relative_loss_per_hour.mean('time')
- if not (loss_value > 0).any():
+ if not np.any(loss_value.values > 0):
return soc_boundary_per_timestep🤖 Prompt for AI Agents
In @flixopt/transform_accessor.py around lines 1293 - 1334, The boolean check on
the xarray DataArray loss_value inside _apply_soc_decay is ambiguous; replace
the current if not (loss_value > 0).any(): with a concrete scalar check by
converting the DataArray to a numpy/scalar before testing (e.g., use
loss_value.values.any() or bool((loss_value > 0).values.any()) or
loss_value.any().item()), so the early return when self-discharge is zero works
correctly and avoids the ValueError raised by evaluating a DataArray in a
boolean context.
| # Use the new transform.cluster() API | ||
| # Note: time_series_for_high_peaks/low_peaks expect string labels matching dataset variables | ||
| time_series_for_high_peaks = ['Wärmelast(Q_th_Last)|fixed_relative_profile'] if keep_extreme_periods else None | ||
| time_series_for_low_peaks = ( | ||
| ['Stromlast(P_el_Last)|fixed_relative_profile', 'Wärmelast(Q_th_Last)|fixed_relative_profile'] | ||
| if keep_extreme_periods | ||
| else None | ||
| ) | ||
|
|
||
| clustered_fs = flow_system.copy().transform.cluster( | ||
| n_clusters=n_clusters, | ||
| cluster_duration=cluster_duration, | ||
| time_series_for_high_peaks=time_series_for_high_peaks, | ||
| time_series_for_low_peaks=time_series_for_low_peaks, | ||
| ) | ||
| clustered_fs.optimize(fx.solvers.HighsSolver(0.01 / 100, 60)) | ||
|
|
||
| # Wrap in a simple object for compatibility with comparison code | ||
| class ClusteredResult: | ||
| def __init__(self, name, fs): | ||
| self.name = name | ||
| self.flow_system = fs | ||
| self.durations = {'total': 0} # Placeholder | ||
|
|
||
| optimization = ClusteredResult('Clustered', clustered_fs) | ||
| optimizations.append(optimization) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for fixed_relative_profile usage
rg -n "fixed_relative_profile" --type=py -A2 -B2Repository: flixOpt/flixopt
Length of output: 48067
🏁 Script executed:
#!/bin/bash
# Search for ClusteredResult or similar wrapper classes
rg -n "class.*Result|class.*Clustered" --type=py -A5Repository: flixOpt/flixopt
Length of output: 7547
🏁 Script executed:
#!/bin/bash
# Look for time_series labels or dataset variable definitions
rg -n "time_series_for_high_peaks|time_series_for_low_peaks" --type=py -B2 -A2Repository: flixOpt/flixopt
Length of output: 7719
🏁 Script executed:
#!/bin/bash
# Search for the transform.cluster API definition
rg -n "def cluster" --type=py -A10Repository: flixOpt/flixopt
Length of output: 5257
🏁 Script executed:
#!/bin/bash
# Look for solve duration capture in optimization code
rg -n "solve.*duration|duration.*solve|elapsed.*time" --type=py -iRepository: flixOpt/flixopt
Length of output: 608
Time-series labels are correct; capture actual optimize() duration in the wrapper.
The time-series label format 'Wärmelast(Q_th_Last)|fixed_relative_profile' correctly matches the dataset variable names and follows the ComponentLabel(FlowLabel)|fixed_relative_profile pattern used throughout the codebase.
However, the ClusteredResult wrapper should capture the actual solve time instead of using a placeholder:
Replace the wrapper with:
# Wrap in a simple object for compatibility with comparison code
class ClusteredResult:
def __init__(self, name, fs, optimization):
self.name = name
self.flow_system = fs
self.durations = optimization.durations # Capture actual solve timeThen pass the optimization object: ClusteredResult('Clustered', clustered_fs, optimization_result)
Otherwise the speed comparison plot will incorrectly show 0 seconds for the clustered approach.
🤖 Prompt for AI Agents
In
@tests/deprecated/examples/03_Optimization_modes/example_optimization_modes.py
around lines 192 - 217, The wrapper currently uses a placeholder duration;
change the optimize call to capture its return value (e.g., optimization_result
= clustered_fs.optimize(...)), update the ClusteredResult class signature to
accept the optimization object (def __init__(self, name, fs, optimization):) and
set self.durations = optimization.durations, and then instantiate with
ClusteredResult('Clustered', clustered_fs, optimization_result) so the actual
solve time is preserved.
Merged main branch updates while preserving property-based architecture: - Kept private attributes with property getters/setters for Storage, OnOffParameters - Added cluster_mode property to OnOffParameters - Took main's improvements: _select_dims helper, join='outer' for xr.concat - Updated CITATION.cff version to 6.0.0rc2 - Added inputs property to Comparison class - Updated dim_priority with 'case' dimension 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Documentation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.