From b393d87a865688fe769d088fa23b989263f55bd3 Mon Sep 17 00:00:00 2001 From: FBumann <117816358+FBumann@users.noreply.github.com> Date: Mon, 27 Jul 2026 16:43:28 +0200 Subject: [PATCH 1/2] fix: load clustered NetCDF files written before flixopt 7.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Files written before 7.0 serialize the clustering under a `results` key holding {'dim_names': [...], 'results': {key: tsam_blob}}, where each key is the slice coordinates joined into a string ('2030|low', or '__single__' when the clustering is undivided). Clustering now expects a `clustering_result` matching tsam_xarray's ClusteringResult.from_dict, which takes a list of {'key': [...], 'clustering': tsam_blob} entries, so loading any pre-7.0 clustered file fails with: Failed to create instance of Clustering: Clustering.__init__() got unexpected keyword arguments: {'results'} The per-slice tsam blobs are byte-identical between the two layouts, so only the surrounding structure is rewritten. Two details matter: - Legacy keys are strings, but the current schema indexes clusterings by the coordinate values, so '2030' has to become the integer 2030 or every lookup misses. The value is recovered by matching against the restored coordinate rather than by guessing a type, so scenario labels that merely look numeric survive unchanged. - Slice dims are stored under their pre-rename spelling, so 'period' becomes '_period' to match what tsam_xarray is handed today. Verified against files generated by flixopt 6.1.0 and 6.2.1 — plain, solved, multi-period, and period+scenario. Cluster assignments compare equal per slice, and an expanded solved file reproduces each original cluster from its typical cluster. The multi-dim fixture moves to module scope so the new tests can reuse its deliberately-different per-slice assignments, which is what catches keys landing on the wrong slice. Co-Authored-By: Claude Opus 5 (1M context) --- flixopt/io.py | 82 ++++++++ tests/test_clustering/test_clustering_io.py | 196 +++++++++++++++----- 2 files changed, 236 insertions(+), 42 deletions(-) diff --git a/flixopt/io.py b/flixopt/io.py index 3d5488404..5197e07fd 100644 --- a/flixopt/io.py +++ b/flixopt/io.py @@ -1846,6 +1846,7 @@ def _restore_clustering( # such files remain loadable. clustering_structure.pop('_original_data_refs', None) clustering_structure.pop('_metrics_refs', None) + clustering_structure = cls._migrate_legacy_clustering_result(clustering_structure, flow_system) clustering = fs_cls._resolve_reference_structure(clustering_structure, {}) flow_system.clustering = clustering @@ -1853,6 +1854,87 @@ def _restore_clustering( if hasattr(clustering, 'cluster_occurrences'): flow_system.cluster_weight = clustering.cluster_occurrences.rename('cluster_weight') + LEGACY_SLICE_DIM_RENAMES = {'period': '_period', 'cluster': '_cluster'} + LEGACY_KEY_SEPARATOR = '|' + + @classmethod + def _migrate_legacy_clustering_result( + cls, + clustering_structure: dict[str, Any], + flow_system: FlowSystem, + ) -> dict[str, Any]: + """Convert a pre-7.0 ``clustering.results`` blob to the current schema. + + Files written before flixopt 7.0 stored the clustering under a ``results`` + key holding ``{'dim_names': [...], 'results': {key: tsam_blob}}``, where the + keys are the slice coordinates joined into a string (``'__single__'`` when + undivided). ``Clustering`` now expects a ``clustering_result`` matching + ``tsam_xarray.ClusteringResult.from_dict``, which takes a list of + ``{'key': [...], 'clustering': tsam_blob}`` entries. The per-slice tsam blobs + themselves are unchanged, so only the surrounding structure is rewritten. + + Args: + clustering_structure: Deserialized ``clustering`` attribute. + flow_system: Partially restored FlowSystem, used to recover the original + dtype of the slice coordinates that the legacy keys stringified. + + Returns: + The structure, converted in place when it used the legacy layout. + """ + if 'clustering_result' in clustering_structure or 'results' not in clustering_structure: + return clustering_structure + + legacy = clustering_structure.pop('results') + legacy_dims = list(legacy.get('dim_names') or []) + + clusterings = [ + { + 'key': cls._restore_legacy_clustering_key(key, legacy_dims, flow_system), + 'clustering': blob, + } + for key, blob in legacy.get('results', {}).items() + ] + + clustering_structure['clustering_result'] = { + 'time_dim': 'time', + 'cluster_dim': ['variable'], + 'slice_dims': [cls.LEGACY_SLICE_DIM_RENAMES.get(dim, dim) for dim in legacy_dims], + 'clusterings': clusterings, + } + return clustering_structure + + @staticmethod + def _restore_legacy_clustering_key( + key: str, + legacy_dims: list[str], + flow_system: FlowSystem, + ) -> list[Any]: + """Turn a stringified legacy clustering key back into coordinate values. + + Legacy keys are strings ('2030', '2030|lo'), but the current schema indexes + clusterings by the coordinate values themselves, so '2030' has to become the + integer 2030 to match a period index. The value is recovered by matching + against the restored coordinate rather than guessing a type, so labels that + merely look numeric survive unchanged. + """ + if not legacy_dims: + return [] + + # Only split when several dims share the key, so single-dim labels + # containing the separator stay intact. + parts = key.split(FlowSystemDatasetIO.LEGACY_KEY_SEPARATOR) if len(legacy_dims) > 1 else [key] + + restored: list[Any] = [] + for dim, part in zip(legacy_dims, parts, strict=False): + index = getattr(flow_system, f'{dim}s', None) + match = next((value for value in index if str(value) == part), None) if index is not None else None + if match is None: + restored.append(part) + else: + # numpy scalars would keep the key untypeable for later lookups + restored.append(match.item() if hasattr(match, 'item') else match) + return restored + @staticmethod def _restore_metadata( flow_system: FlowSystem, diff --git a/tests/test_clustering/test_clustering_io.py b/tests/test_clustering/test_clustering_io.py index e96468eb1..edabf7383 100644 --- a/tests/test_clustering/test_clustering_io.py +++ b/tests/test_clustering/test_clustering_io.py @@ -521,50 +521,51 @@ def test_clustering_preserves_component_labels(self, simple_system_8_days, solve assert 'source' in fs_expanded.components -class TestMultiDimensionalClusteringIO: - """Test IO for clustering with both periods and scenarios (multi-dimensional).""" +@pytest.fixture +def system_with_periods_and_scenarios(): + """Create a flow system with both periods and scenarios, with different demand patterns.""" + n_days = 3 + hours = 24 * n_days + timesteps = pd.date_range('2024-01-01', periods=hours, freq='h') + periods = pd.Index([2024, 2025], name='period') + scenarios = pd.Index(['high', 'low'], name='scenario') + + # Create DIFFERENT demand patterns per period/scenario to get different cluster assignments + # Pattern structure: (base_mean, amplitude) for each day + patterns = { + (2024, 'high'): [(100, 40), (100, 40), (50, 20)], # Days 0&1 similar + (2024, 'low'): [(50, 20), (100, 40), (100, 40)], # Days 1&2 similar + (2025, 'high'): [(100, 40), (50, 20), (100, 40)], # Days 0&2 similar + (2025, 'low'): [(50, 20), (50, 20), (100, 40)], # Days 0&1 similar + } + + demand_values = np.zeros((hours, len(periods), len(scenarios))) + for pi, period in enumerate(periods): + for si, scenario in enumerate(scenarios): + base = np.zeros(hours) + for d, (mean, amp) in enumerate(patterns[(period, scenario)]): + start = d * 24 + base[start : start + 24] = mean + amp * np.sin(np.linspace(0, 2 * np.pi, 24)) + demand_values[:, pi, si] = base + + demand = xr.DataArray( + demand_values, + dims=['time', 'period', 'scenario'], + coords={'time': timesteps, 'period': periods, 'scenario': scenarios}, + ) - @pytest.fixture - def system_with_periods_and_scenarios(self): - """Create a flow system with both periods and scenarios, with different demand patterns.""" - n_days = 3 - hours = 24 * n_days - timesteps = pd.date_range('2024-01-01', periods=hours, freq='h') - periods = pd.Index([2024, 2025], name='period') - scenarios = pd.Index(['high', 'low'], name='scenario') - - # Create DIFFERENT demand patterns per period/scenario to get different cluster assignments - # Pattern structure: (base_mean, amplitude) for each day - patterns = { - (2024, 'high'): [(100, 40), (100, 40), (50, 20)], # Days 0&1 similar - (2024, 'low'): [(50, 20), (100, 40), (100, 40)], # Days 1&2 similar - (2025, 'high'): [(100, 40), (50, 20), (100, 40)], # Days 0&2 similar - (2025, 'low'): [(50, 20), (50, 20), (100, 40)], # Days 0&1 similar - } - - demand_values = np.zeros((hours, len(periods), len(scenarios))) - for pi, period in enumerate(periods): - for si, scenario in enumerate(scenarios): - base = np.zeros(hours) - for d, (mean, amp) in enumerate(patterns[(period, scenario)]): - start = d * 24 - base[start : start + 24] = mean + amp * np.sin(np.linspace(0, 2 * np.pi, 24)) - demand_values[:, pi, si] = base - - demand = xr.DataArray( - demand_values, - dims=['time', 'period', 'scenario'], - coords={'time': timesteps, 'period': periods, 'scenario': scenarios}, - ) + fs = fx.FlowSystem(timesteps, periods=periods, scenarios=scenarios) + fs.add_elements( + fx.Bus('heat'), + fx.Effect('costs', unit='EUR', description='costs', is_objective=True, is_standard=True), + fx.Sink('demand', inputs=[fx.Flow('in', bus='heat', fixed_relative_profile=demand, size=1)]), + fx.Source('source', outputs=[fx.Flow('out', bus='heat', size=200, effects_per_flow_hour={'costs': 0.05})]), + ) + return fs - fs = fx.FlowSystem(timesteps, periods=periods, scenarios=scenarios) - fs.add_elements( - fx.Bus('heat'), - fx.Effect('costs', unit='EUR', description='costs', is_objective=True, is_standard=True), - fx.Sink('demand', inputs=[fx.Flow('in', bus='heat', fixed_relative_profile=demand, size=1)]), - fx.Source('source', outputs=[fx.Flow('out', bus='heat', size=200, effects_per_flow_hour={'costs': 0.05})]), - ) - return fs + +class TestMultiDimensionalClusteringIO: + """Test IO for clustering with both periods and scenarios (multi-dimensional).""" def test_cluster_assignments_has_correct_dimensions(self, system_with_periods_and_scenarios): """cluster_assignments should have dimensions for original_cluster, period, and scenario.""" @@ -761,3 +762,114 @@ def test_load_legacy_clustered_netcdf(self, simple_system_8_days, tmp_path): assert isinstance(fs_restored.clustering, Clustering) assert fs_restored.clustering.n_clusters == 2 + + +class TestPreV7ClusteringSchema: + """Loading clustered files written before flixopt 7.0, which serialized the + clustering under a ``results`` key with string-joined slice keys instead of the + ``clustering_result`` schema that tsam_xarray's ClusteringResult now expects. + + The per-slice tsam blobs are unchanged between the two layouts, so these tests + rewrite a current dataset into the legacy layout rather than shipping a binary + fixture. The layout was verified against files generated by flixopt 6.2.1. + """ + + def _to_legacy_schema(self, ds: xr.Dataset) -> xr.Dataset: + """Rewrite a clustered dataset's clustering attrs into the pre-7.0 layout.""" + import json + + clustering = json.loads(ds.attrs['clustering']) + result = clustering.pop('clustering_result') + + unrename = {'_period': 'period', '_cluster': 'cluster'} + dim_names = [unrename.get(dim, dim) for dim in result['slice_dims']] + + legacy_results = {} + for entry in result['clusterings']: + key = '|'.join(str(part) for part in entry['key']) if entry['key'] else '__single__' + legacy_results[key] = entry['clustering'] + + clustering['results'] = {'dim_names': dim_names, 'results': legacy_results} + # Pre-7.0 files always carried these; they are dropped on load. + clustering['_original_data_refs'] = [] + clustering['_metrics_refs'] = None + ds.attrs['clustering'] = json.dumps(clustering, ensure_ascii=False) + return ds + + def _assignments(self, flow_system) -> xr.DataArray: + return flow_system.clustering.cluster_assignments + + def test_legacy_schema_is_rejected_without_migration(self, simple_system_8_days): + """Sanity check: the legacy key really is incompatible with Clustering.__init__.""" + from flixopt.clustering import Clustering + + fs_clustered = simple_system_8_days.transform.cluster(n_clusters=2, cluster_duration='1D') + ds = self._to_legacy_schema(fs_clustered.to_dataset(include_solution=False)) + + import json + + legacy = json.loads(ds.attrs['clustering']) + assert 'results' in legacy and 'clustering_result' not in legacy + with pytest.raises(TypeError): + Clustering(results=legacy['results']) + + def test_load_legacy_schema_preserves_assignments(self, simple_system_8_days): + """A pre-7.0 clustered dataset loads with its cluster assignments intact.""" + from flixopt.clustering import Clustering + + fs_clustered = simple_system_8_days.transform.cluster(n_clusters=2, cluster_duration='1D') + expected = self._assignments(fs_clustered).values.copy() + ds = self._to_legacy_schema(fs_clustered.to_dataset(include_solution=False)) + + fs_restored = fx.FlowSystem.from_dataset(ds) + + assert isinstance(fs_restored.clustering, Clustering) + assert fs_restored.clustering.n_clusters == 2 + np.testing.assert_array_equal(self._assignments(fs_restored).values, expected) + + def test_load_legacy_schema_netcdf_roundtrip(self, simple_system_8_days, tmp_path): + """Same, but through a real NetCDF file rather than an in-memory dataset.""" + from flixopt.io import save_dataset_to_netcdf + + fs_clustered = simple_system_8_days.transform.cluster(n_clusters=2, cluster_duration='1D') + expected = self._assignments(fs_clustered).values.copy() + ds = self._to_legacy_schema(fs_clustered.to_dataset(include_solution=False)) + + nc_path = tmp_path / 'legacy_schema.nc' + save_dataset_to_netcdf(ds, nc_path) + fs_restored = fx.FlowSystem.from_netcdf(nc_path) + + np.testing.assert_array_equal(self._assignments(fs_restored).values, expected) + + def test_legacy_schema_expands_to_full_timesteps(self, simple_system_8_days): + """A loaded pre-7.0 file can still be expanded back to the original timesteps.""" + fs_clustered = simple_system_8_days.transform.cluster(n_clusters=2, cluster_duration='1D') + ds = self._to_legacy_schema(fs_clustered.to_dataset(include_solution=False)) + + fs_expanded = fx.FlowSystem.from_dataset(ds).transform.expand() + + assert len(fs_expanded.timesteps) == 8 * 24 + + def test_legacy_multi_dim_keys_map_to_the_right_slice(self, system_with_periods_and_scenarios): + """Legacy keys are strings ('2024|high'); each must land on its own slice. + + Sensitivity: if the string keys were parsed without recovering the original + coordinate dtype, every lookup would miss and the assignments would silently + collapse onto one slice or swap between periods. + """ + fs_clustered = system_with_periods_and_scenarios.transform.cluster(n_clusters=2, cluster_duration='1D') + expected = self._assignments(fs_clustered) + ds = self._to_legacy_schema(fs_clustered.to_dataset(include_solution=False)) + + fs_restored = fx.FlowSystem.from_dataset(ds) + restored = self._assignments(fs_restored) + + assert set(restored.dims) == set(expected.dims) + for period in fs_restored.periods: + for scenario in fs_restored.scenarios: + sel = {'period': period, 'scenario': scenario} + np.testing.assert_array_equal( + restored.sel(sel).values, + expected.sel(sel).values, + err_msg=f'assignments differ for {sel}', + ) From 021cf19a2c3d0379af8a0737b67e04b2ef915253 Mon Sep 17 00:00:00 2001 From: FBumann <117816358+FBumann@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:04:46 +0200 Subject: [PATCH 2/2] refactor: fold legacy clustering key restoration into one pass The key restoration was a second method scanning the coordinate index once per key part. Building a stringified lookup per slice dim up front collapses it into the migration itself, at the same cost. Behaviour is unchanged: 291 clustering tests pass, and files generated by flixopt 6.1.0 and 6.2.1 (plain, solved, multi-period, period+scenario) still load with assignments equal per slice. Co-Authored-By: Claude Opus 5 (1M context) --- flixopt/io.py | 85 +++++++++++++++++++-------------------------------- 1 file changed, 31 insertions(+), 54 deletions(-) diff --git a/flixopt/io.py b/flixopt/io.py index 5197e07fd..2e21aa0ef 100644 --- a/flixopt/io.py +++ b/flixopt/io.py @@ -1865,76 +1865,53 @@ def _migrate_legacy_clustering_result( ) -> dict[str, Any]: """Convert a pre-7.0 ``clustering.results`` blob to the current schema. - Files written before flixopt 7.0 stored the clustering under a ``results`` - key holding ``{'dim_names': [...], 'results': {key: tsam_blob}}``, where the - keys are the slice coordinates joined into a string (``'__single__'`` when + Files written before flixopt 7.0 stored the clustering as + ``{'dim_names': [...], 'results': {key: tsam_blob}}``, keyed by the slice + coordinates joined into a string (``'2030|low'``, or ``'__single__'`` when undivided). ``Clustering`` now expects a ``clustering_result`` matching ``tsam_xarray.ClusteringResult.from_dict``, which takes a list of - ``{'key': [...], 'clustering': tsam_blob}`` entries. The per-slice tsam blobs - themselves are unchanged, so only the surrounding structure is rewritten. - - Args: - clustering_structure: Deserialized ``clustering`` attribute. - flow_system: Partially restored FlowSystem, used to recover the original - dtype of the slice coordinates that the legacy keys stringified. - - Returns: - The structure, converted in place when it used the legacy layout. + ``{'key': [...], 'clustering': tsam_blob}`` entries and indexes them by the + coordinate values themselves -- so ``'2030'`` has to become the integer 2030 + or every lookup silently misses. The values are recovered by matching against + the coordinates already restored on ``flow_system``, rather than by guessing a + type, so labels that merely look numeric survive unchanged. + + The per-slice tsam blobs are identical between the two layouts; only the + structure around them is rewritten. """ if 'clustering_result' in clustering_structure or 'results' not in clustering_structure: return clustering_structure legacy = clustering_structure.pop('results') - legacy_dims = list(legacy.get('dim_names') or []) + dims = list(legacy.get('dim_names') or []) - clusterings = [ - { - 'key': cls._restore_legacy_clustering_key(key, legacy_dims, flow_system), - 'clustering': blob, - } - for key, blob in legacy.get('results', {}).items() - ] + lookups = [] + for dim in dims: + index = getattr(flow_system, f'{dim}s', None) + values = [] if index is None else list(index) + # .item() unwraps numpy scalars so the keys stay plain Python values + lookups.append({str(value): value.item() if hasattr(value, 'item') else value for value in values}) + + clusterings = [] + for key, blob in legacy.get('results', {}).items(): + # Split only when several dims share the key, so single-dim labels + # containing the separator stay intact. + parts = key.split(cls.LEGACY_KEY_SEPARATOR) if len(dims) > 1 else [key] + clusterings.append( + { + 'key': [lookup.get(part, part) for lookup, part in zip(lookups, parts, strict=False)], + 'clustering': blob, + } + ) clustering_structure['clustering_result'] = { 'time_dim': 'time', 'cluster_dim': ['variable'], - 'slice_dims': [cls.LEGACY_SLICE_DIM_RENAMES.get(dim, dim) for dim in legacy_dims], + 'slice_dims': [cls.LEGACY_SLICE_DIM_RENAMES.get(dim, dim) for dim in dims], 'clusterings': clusterings, } return clustering_structure - @staticmethod - def _restore_legacy_clustering_key( - key: str, - legacy_dims: list[str], - flow_system: FlowSystem, - ) -> list[Any]: - """Turn a stringified legacy clustering key back into coordinate values. - - Legacy keys are strings ('2030', '2030|lo'), but the current schema indexes - clusterings by the coordinate values themselves, so '2030' has to become the - integer 2030 to match a period index. The value is recovered by matching - against the restored coordinate rather than guessing a type, so labels that - merely look numeric survive unchanged. - """ - if not legacy_dims: - return [] - - # Only split when several dims share the key, so single-dim labels - # containing the separator stay intact. - parts = key.split(FlowSystemDatasetIO.LEGACY_KEY_SEPARATOR) if len(legacy_dims) > 1 else [key] - - restored: list[Any] = [] - for dim, part in zip(legacy_dims, parts, strict=False): - index = getattr(flow_system, f'{dim}s', None) - match = next((value for value in index if str(value) == part), None) if index is not None else None - if match is None: - restored.append(part) - else: - # numpy scalars would keep the key untypeable for later lookups - restored.append(match.item() if hasattr(match, 'item') else match) - return restored - @staticmethod def _restore_metadata( flow_system: FlowSystem,