feat!: rename label → id, add IdList[T], redesign Flow constructor#610
feat!: rename label → id, add IdList[T], redesign Flow constructor#610FBumann wants to merge 3 commits intofeature/element-data-classesfrom
label → id, add IdList[T], redesign Flow constructor#610Conversation
…ow` constructor - Rename `label` to `id` on all Element subclasses with deprecation bridge (both `label=` kwarg and `.label`/`.label_full` properties still work with DeprecationWarning, to be removed in v7.0.0) - Add `IdList[T]` generic frozen ordered container (`flixopt/id_list.py`) replacing FlowContainer, ElementContainer, ResultsContainer, and CarrierContainer. Supports primary key, short-key fallback, and positional index access with close-match suggestions on errors. - Redesign Flow constructor: `bus` is first positional arg, `id` defaults to bus name. Old forms `Flow(label, bus)` still work via deprecation bridge. - Flow.id now returns qualified `component(short_id)` form. Conversion factor dict keys use `_short_id` for internal consistency. - Handle old serialized data: `label` → `id` conversion during deserialization, `_short_id` used for serialization to avoid qualified names in stored data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a comprehensive refactoring that replaces label-based identifiers with id-based identifiers throughout the codebase. It adds a new generic Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
FBumann
changed the title
label → id, add IdList[T] container, redesign `Fl…label → id, add IdList[T], redesign Flow constructor
1. Implemented flow_id as public field on Flow - Flow constructor: id param renamed to flow_id (optional, defaults to bus name) - Flow.flow_id property: public access to the short name (was _short_id) - Flow.id property: returns qualified component(flow_id) (unchanged behavior) - Deprecation bridge: Flow(id=...) now warns → maps to flow_id; Flow(label=...) also maps to flow_id - Serialization: flow_id correctly serialized/deserialized; id on Flow skipped as deprecated - Updated all internal _short_id references on Flow instances → flow_id: id_list.py, linear_converters.py, topology_accessor.py, batched.py, elements.py 2. Updated all tests to use new API - 7 test files updated: conftest.py, test_scenarios.py, test_flow_system_resample.py, test_solution_and_plotting.py, test_functional.py, test_linear_converter.py, test_flow.py - All Flow(name, bus=...) → Flow(bus, flow_id=name, ...) - All Flow(label=name, bus=...) → Flow(bus, flow_id=name, ...) - All Element(label=name) → Element(name) or Element(id=name) 3. Results: 1181 passed, 127 skipped, 0 failures (with strict deprecation warnings)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (7)
tests/test_scenarios.py (1)
59-96:⚠️ Potential issue | 🟡 MinorAvoid deprecated
Bus.label_fullinFlow(...)bus argument (useBus.id/ raw string).Given the label→id migration,
label_fullis a deprecation bridge and may emit warnings; these fixtures can pass the busiddirectly. This also makes it clearer that the first positional arg is the bus (not the flow identifier).Proposed diff
- demand = Flow(electricity_bus.label_full, flow_id='Demand', fixed_relative_profile=demand_profiles) + demand = Flow(electricity_bus.id, flow_id='Demand', fixed_relative_profile=demand_profiles) power_gen = Flow( - electricity_bus.label_full, + electricity_bus.id, flow_id='Generation', size=InvestParameters( minimum_size=0, maximum_size=20, effects_of_investment_per_size={'costs': 100}, # €/kW ), effects_per_flow_hour={'costs': 20}, # €/MWh ) - storage_charge = Flow(electricity_bus.label_full, flow_id='Charge', size=10) - storage_discharge = Flow(electricity_bus.label_full, flow_id='Discharge', size=10) + storage_charge = Flow(electricity_bus.id, flow_id='Charge', size=10) + storage_discharge = Flow(electricity_bus.id, flow_id='Discharge', size=10)flixopt/linear_converters.py (1)
375-414:⚠️ Potential issue | 🔴 CriticalCoolingTower:
specific_electricity_demandis effectively required but not validated (None will break).
__init__allowsspecific_electricity_demand=None, but the setter immediately callscheck_bounds()which will error onNone(np.asarray(None)comparisons). This should be validated like the other required parameters.Proposed diff
class CoolingTower(LinearConverter): @@ def __init__( self, id: str | None = None, specific_electricity_demand: Numeric_TPS | None = None, electrical_flow: Flow | None = None, thermal_flow: Flow | None = None, @@ ): # Validate required parameters if electrical_flow is None: raise ValueError(f"'{id}': electrical_flow is required and cannot be None") if thermal_flow is None: raise ValueError(f"'{id}': thermal_flow is required and cannot be None") + if specific_electricity_demand is None: + raise ValueError(f"'{id}': specific_electricity_demand is required and cannot be None")flixopt/topology_accessor.py (1)
414-432:⚠️ Potential issue | 🟠 MajorFix
set_carrier_color: useself._fs.carriers.get()instead ofFlowSystem.get_carrier()
FlowSystem.get_carrier()is designed for bus/flow ids (e.g., 'Fernwärme', 'Boiler(Q_th)'), not carrier names. The docstring explicitly recommends usingflow_system.carriers['electricity']for direct carrier lookups by name. The current implementation will fail when passing a carrier name like 'electricity'. Carriers are keyed by lowercase name in the carriers dict.Proposed fix
def set_carrier_color(self, carrier: str, color: str) -> None: """Set the color for a carrier. This affects bus colors derived from this carrier. Args: carrier: Carrier name (case-insensitive). color: Color string (hex like '#FF0000', named like 'red', etc.). Examples: - carrier_obj = self._fs.get_carrier(carrier) + carrier_obj = self._fs.carriers.get(carrier.lower()) if carrier_obj is None: raise KeyError(f"Carrier '{carrier}' not found.") carrier_obj.color = colorflixopt/flow_system.py (1)
1037-1066:⚠️ Potential issue | 🔴 Critical
get_carrier()API shift breaksTopologyAccessor.set_carrier_color()— must update to pass carrier name via direct dictionary access.The method
set_carrier_color(carrier: str, color: str)atflixopt/topology_accessor.py:414currently callsself._fs.get_carrier(carrier)at line 427, passing a carrier name like'electricity'. After the API change,get_carrier()now expects bus or flow IDs, not carrier names. This causesset_carrier_color('electricity')to returnNoneand raiseKeyErrorincorrectly.Fix by replacing the
get_carrier()call with direct access:self._fs.carriers[carrier.lower()](matching the pattern inget_carrier()at line 1057), and handle theKeyErrorappropriately if the carrier doesn't exist.tests/flow_system/test_flow_system_resample.py (1)
160-163:⚠️ Potential issue | 🟡 MinorReplace deprecated
.labelassertions withflow_id(orid).The
.labelproperty is deprecated and may raise warnings that break tests under strict warning settings.✅ Suggested change
- assert storage.charging.label == 'charge' - assert storage.discharging.label == 'discharge' + assert storage.charging.flow_id == 'charge' + assert storage.discharging.flow_id == 'discharge'flixopt/results.py (1)
256-293:⚠️ Potential issue | 🔴 CriticalFix schema mismatch: infos dicts have
idkey but constructors expectlabelparameter.The solution attrs store
{'id': ..., 'variables': ..., ...}(per structure.py lines 844–879), but unpacking with**infosinto constructors expecting alabelparameter will raiseTypeError: __init__() got an unexpected keyword argument 'id'. The constructors for_ElementResults,_NodeResults, andFlowResultsall acceptlabelas their second parameter, notid.Normalization helper
+ def _normalize_infos(infos: dict) -> dict: + if 'id' in infos and 'label' not in infos: + infos = dict(infos) + infos['label'] = infos.pop('id') + return infos + components_dict = { - label: ComponentResults(self, **infos) + label: ComponentResults(self, **_normalize_infos(infos)) for label, infos in _get_solution_attr(self.solution, 'Components').items() } buses_dict = { - label: BusResults(self, **infos) for label, infos in _get_solution_attr(self.solution, 'Buses').items() + label: BusResults(self, **_normalize_infos(infos)) for label, infos in _get_solution_attr(self.solution, 'Buses').items() } effects_dict = { - label: EffectResults(self, **infos) for label, infos in _get_solution_attr(self.solution, 'Effects').items() + label: EffectResults(self, **_normalize_infos(infos)) for label, infos in _get_solution_attr(self.solution, 'Effects').items() } - flows_dict = {label: FlowResults(self, **infos) for label, infos in flows_attr.items()} + flows_dict = {label: FlowResults(self, **_normalize_infos(infos)) for label, infos in flows_attr.items()}flixopt/elements.py (1)
2534-2539:⚠️ Potential issue | 🟠 MajorGuard absolute‑loss terms to avoid NaNs when only some bidirectional transmissions have status.
If
FlowVarName.STATUSis masked (linopy) for flows without status,(flow_status * d.in2_mask).sum('flow')can yield NaNs for transmissions without abs-losses. Subtracting that across all bidirectional transmissions risks contaminating constraints. Consider applying the abs-loss subtraction only tobidir_with_absor verifying that masked entries are zeros.🛠️ Possible masking approach
bidir_with_abs = [t.id for t in d.bidirectional if t.id in d.transmissions_with_abs_losses] if bidir_with_abs: flow_status = self._flows_model[FlowVarName.STATUS] - in2_status = (flow_status * d.in2_mask).sum('flow') - efficiency_expr_2 = efficiency_expr_2 - in2_status * abs_losses_bidir + abs_mask = d.in2_mask.sel({self.dim_name: bidir_with_abs}) + in2_status = (flow_status * abs_mask).sum('flow') + efficiency_expr_2 = efficiency_expr_2.copy() + efficiency_expr_2.loc[{self.dim_name: bidir_with_abs}] = ( + efficiency_expr_2.sel({self.dim_name: bidir_with_abs}) + - in2_status * abs_losses_bidir.sel({self.dim_name: bidir_with_abs}) + )#!/bin/bash # Inspect how absolute_losses / transmissions_with_abs_losses are built, # and whether non-abs-loss transmissions get 0s or NaNs. rg -n "absolute_losses|transmissions_with_abs_losses|with_abs_losses" --type py
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 65-66: Update the v7.0.0 changelog wording to avoid implying
removal in the same release: change the deprecation removal target for the old
`label` parameter and `.label` / `.label_full` properties (referenced in the
`label` → `id` note) from "v7.0.0" to either "v8.0.0" or "a future major
release"; likewise clarify the `Flow` constructor note (mentions `Flow(label,
bus)` compatibility) to say those old forms remain deprecated until that future
major release. Ensure references to symbols `label`, `id`, `Flow`, `.label`, and
`.label_full` are updated consistently.
In `@flixopt/effects.py`:
- Around line 219-223: The penalty-objective guard currently compares the
passed-in id parameter instead of the resolved effect id, so when a deprecated
label is used (id=None) the check can be bypassed; move or perform the check
after calling super().__init__ and compare against self.id (not the id argument)
and PENALTY_EFFECT_ID when is_objective is true to ensure the final resolved id
is validated (refer to the constructor where super().__init__ is called and the
variables is_objective, id, self.id, and PENALTY_EFFECT_ID).
In `@flixopt/elements.py`:
- Around line 806-826: The setters for flow_id and id currently assign directly
to self._short_id and bypass _valid_id; update the flow_id.setter and id.setter
to validate the incoming value by calling self._valid_id(value) (or the
appropriate validation helper) and raise a ValueError with a clear message if
validation fails, otherwise assign to self._short_id; ensure id.setter still
preserves the intended format (accepting the same short identifier string used
elsewhere) and reuse the same validation logic as flow_id.setter so the ID
invariants are enforced post-init.
In `@flixopt/flow_system.py`:
- Around line 232-240: The docstrings for scenario_independent_sizes and
scenario_independent_flow_rates are ambiguous about what IDs the lists should
contain; update the descriptions and type hints so they explicitly state that
lists must contain investable flow/storage IDs (not generic component names) —
e.g., qualified component identifiers such as "Component(flow_id)" or the
flow/storage id string — and clarify that scenario_independent_sizes targets
investable flows/storages (sizes/investments) whereas
scenario_independent_flow_rates targets flow rate IDs; apply this wording update
to every docstring mentioning these params (including the other occurrences
noted) and include a short example of the expected string format.
In `@flixopt/id_list.py`:
- Around line 104-108: The current short-key fallback loop using
self._short_key_fn over self._data.values() returns the first match and is
nondeterministic when multiple elements share the same short key; change the
logic in the lookup (where _short_key_fn and _data are used) to collect all
elements whose _short_key_fn(elem) == key, and if more than one match is found
raise a clear ValueError (or a specific exception) that includes the full
identifiers of the matching elements so callers can resolve the ambiguity
instead of silently returning the first match.
In `@flixopt/io.py`:
- Around line 1239-1257: The docstring for the repr helper is inaccurate: it
says the default excluded params are {'self', 'id', 'kwargs'} but the
implementation of the function (parameters excluded_params, label_as_positional,
id_as_positional) actually always excludes 'label' as well; update the docstring
to list the true defaults (e.g., 'self', 'id', 'label', and 'kwargs') and note
that 'label' is a deprecated alias for 'id' or that label_as_positional is a
deprecated alias for id_as_positional so the documentation matches the behavior
of the code paths that union excluded_params with {'self','label'}.
In `@flixopt/results.py`:
- Around line 100-103: Update the docstring entries that currently say
"Dictionary" to instead state "IdList" (or "mapping-like IdList" where
appropriate) for the fields components, buses, effects, and timesteps_extra so
the documentation reflects the actual type/behavior (ordering, short-key
fallback). Also apply the same replacement for the other occurrences mentioned
around the later docstring block (the entries at the 159-160 area). Reference
the field names components, buses, effects, timesteps_extra and make the wording
consistent across both docstring locations.
In `@flixopt/structure.py`:
- Around line 2031-2043: The id setter currently assigns to _short_id without
validation; update the Element.id setter to call the existing _valid_id (or
validation helper) on the incoming value and raise a ValueError (or appropriate
exception) if it fails, otherwise set self._short_id to the validated value so
invalid characters cannot be introduced after initialization (refer to the
Element.id property/setter and the _valid_id validation function).
🧹 Nitpick comments (5)
flixopt/effects.py (1)
876-882: Effect membership should be id-based for Effect objectsThe current
__contains__returnsFalsefor a different Effect instance with the same id. That’s surprising for a container keyed by id and makes duplicate checks less intuitive. Prefer id-based membership for Effect objects.♻️ Suggested tweak
- elif isinstance(item, Effect): - return item.id in self and self[item.id] is item + elif isinstance(item, Effect): + return super().__contains__(item.id)tests/conftest.py (1)
605-666: Optional: be cautious with short-key flow lookups whenflow_idrepeats across components.This fixture creates multiple flows with the same
flow_id(e.g.,P_el) across different components. That’s fine as long as callers use qualified ids (e.g.,Component(P_el)) when indexingFlowSystem.flows; short-key fallbacks can become ambiguous.flixopt/flow_system.py (1)
1758-1775: Avoid silently overwritingflow.component(can indirectly mutateflow.id, which is used as a key).Since IdList keys are id-based, mutating fields that participate in
flow.idafter the flow is stored anywhere is risky. Consider only settingflow.componentwhen missing, and raising if it contradicts the owning component—this prevents subtle container/key corruption.Proposed diff
for component in self.components.values(): for flow in component.flows.values(): - flow.component = component.id + if flow.component is None: + flow.component = component.id + elif flow.component != component.id: + raise ValueError( + f'Flow "{flow.flow_id}" is already assigned to component "{flow.component}", ' + f'cannot also assign it to "{component.id}".' + ) flow.is_input_in_component = flow.id in component.inputsflixopt/structure.py (1)
439-441: Docstring still referenceslabel_full.
element_idsnow represent ids; updating the docstring avoids confusion.flixopt/components.py (1)
1067-1093: Use storage IDs for the balanced‑pairing dimension.Assigning
charge_idsto both charge and discharge sizes mislabels the coordinate and makes debugging harder. Using storage ids (or explicit pair ids) keeps alignment explicit.♻️ Suggested refactor
- charge_ids = [] - discharge_ids = [] + pair_ids = [] + charge_ids = [] + discharge_ids = [] for sid in balanced_ids: s = self.data[sid] cid = s.charging.id did = s.discharging.id if cid in investment_ids_set and did in investment_ids_set: + pair_ids.append(sid) charge_ids.append(cid) discharge_ids.append(did) @@ balanced_dim = 'balanced_storage' - charge_sizes = charge_sizes.rename({flow_dim: balanced_dim}).assign_coords({balanced_dim: charge_ids}) - discharge_sizes = discharge_sizes.rename({flow_dim: balanced_dim}).assign_coords({balanced_dim: charge_ids}) + charge_sizes = charge_sizes.rename({flow_dim: balanced_dim}).assign_coords({balanced_dim: pair_ids}) + discharge_sizes = discharge_sizes.rename({flow_dim: balanced_dim}).assign_coords({balanced_dim: pair_ids})
CHANGELOG.md
Outdated
| - **`label` renamed to `id`**: All element constructors now use `id` instead of `label`. The old `label` parameter and `.label` / `.label_full` properties are deprecated and will be removed in v7.0.0. Use `.id` everywhere. | ||
| - **`Flow` constructor redesigned**: `bus` is now the first positional argument; `flow_id` (optional) sets the short name, defaulting to the bus name. Old forms `Flow(label, bus)` and `Flow(label, bus=...)` still work with deprecation warnings. |
There was a problem hiding this comment.
Clarify deprecation removal version in 7.0.0 notes
Line 65-66 says deprecated label APIs will be removed in v7.0.0, but this section is the v7.0.0 release and the code keeps them with warnings. Please update to the next major (e.g., v8.0.0) or “future major release” to avoid conflicting guidance.
✏️ Suggested wording
-- **`label` renamed to `id`**: All element constructors now use `id` instead of `label`. The old `label` parameter and `.label` / `.label_full` properties are deprecated and will be removed in v7.0.0. Use `.id` everywhere.
+- **`label` renamed to `id`**: All element constructors now use `id` instead of `label`. The old `label` parameter and `.label` / `.label_full` properties are deprecated and will be removed in v8.0.0. Use `.id` everywhere.📝 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.
| - **`label` renamed to `id`**: All element constructors now use `id` instead of `label`. The old `label` parameter and `.label` / `.label_full` properties are deprecated and will be removed in v7.0.0. Use `.id` everywhere. | |
| - **`Flow` constructor redesigned**: `bus` is now the first positional argument; `flow_id` (optional) sets the short name, defaulting to the bus name. Old forms `Flow(label, bus)` and `Flow(label, bus=...)` still work with deprecation warnings. | |
| - **`label` renamed to `id`**: All element constructors now use `id` instead of `label`. The old `label` parameter and `.label` / `.label_full` properties are deprecated and will be removed in v8.0.0. Use `.id` everywhere. | |
| - **`Flow` constructor redesigned**: `bus` is now the first positional argument; `flow_id` (optional) sets the short name, defaulting to the bus name. Old forms `Flow(label, bus)` and `Flow(label, bus=...)` still work with deprecation warnings. |
🤖 Prompt for AI Agents
In `@CHANGELOG.md` around lines 65 - 66, Update the v7.0.0 changelog wording to
avoid implying removal in the same release: change the deprecation removal
target for the old `label` parameter and `.label` / `.label_full` properties
(referenced in the `label` → `id` note) from "v7.0.0" to either "v8.0.0" or "a
future major release"; likewise clarify the `Flow` constructor note (mentions
`Flow(label, bus)` compatibility) to say those old forms remain deprecated until
that future major release. Ensure references to symbols `label`, `id`, `Flow`,
`.label`, and `.label_full` are updated consistently.
flixopt/effects.py
Outdated
| # Validate that Penalty cannot be set as objective | ||
| if is_objective and label == PENALTY_EFFECT_LABEL: | ||
| if is_objective and id == PENALTY_EFFECT_ID: | ||
| raise ValueError( | ||
| f'The Penalty effect ("{PENALTY_EFFECT_LABEL}") cannot be set as the objective effect. ' | ||
| f'The Penalty effect ("{PENALTY_EFFECT_ID}") cannot be set as the objective effect. ' | ||
| f'Please use a different effect as the optimization objective.' |
There was a problem hiding this comment.
Penalty objective check should use resolved id
Line 219-223 compares against the id argument instead of the resolved self.id. If deprecated label is used (and id=None), the penalty guard can be bypassed. Use self.id after super().__init__ so the check always sees the final id.
🛠️ Suggested fix
- if is_objective and id == PENALTY_EFFECT_ID:
+ if is_objective and self.id == PENALTY_EFFECT_ID:📝 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.
| # Validate that Penalty cannot be set as objective | |
| if is_objective and label == PENALTY_EFFECT_LABEL: | |
| if is_objective and id == PENALTY_EFFECT_ID: | |
| raise ValueError( | |
| f'The Penalty effect ("{PENALTY_EFFECT_LABEL}") cannot be set as the objective effect. ' | |
| f'The Penalty effect ("{PENALTY_EFFECT_ID}") cannot be set as the objective effect. ' | |
| f'Please use a different effect as the optimization objective.' | |
| # Validate that Penalty cannot be set as objective | |
| if is_objective and self.id == PENALTY_EFFECT_ID: | |
| raise ValueError( | |
| f'The Penalty effect ("{PENALTY_EFFECT_ID}") cannot be set as the objective effect. ' | |
| f'Please use a different effect as the optimization objective.' |
🤖 Prompt for AI Agents
In `@flixopt/effects.py` around lines 219 - 223, The penalty-objective guard
currently compares the passed-in id parameter instead of the resolved effect id,
so when a deprecated label is used (id=None) the check can be bypassed; move or
perform the check after calling super().__init__ and compare against self.id
(not the id argument) and PENALTY_EFFECT_ID when is_objective is true to ensure
the final resolved id is validated (refer to the constructor where
super().__init__ is called and the variables is_objective, id, self.id, and
PENALTY_EFFECT_ID).
| @property | ||
| def label_full(self) -> str: | ||
| return f'{self.component}({self.label})' | ||
| def flow_id(self) -> str: | ||
| """The short flow identifier (e.g. ``'Heat'``). | ||
|
|
||
| This is the user-facing name. Defaults to the bus name if not set explicitly. | ||
| """ | ||
| return self._short_id | ||
|
|
||
| @flow_id.setter | ||
| def flow_id(self, value: str) -> None: | ||
| self._short_id = value | ||
|
|
||
| @property | ||
| def id(self) -> str: | ||
| """The qualified identifier: ``component(flow_id)``.""" | ||
| return f'{self.component}({self._short_id})' | ||
|
|
||
| @id.setter | ||
| def id(self, value: str) -> None: | ||
| self._short_id = value | ||
|
|
There was a problem hiding this comment.
Validate flow identifiers in setters to preserve ID invariants.
The setters bypass _valid_id, so invalid characters can slip in post-init and later break naming/serialization. Suggest validating in both flow_id and id setters.
🔧 Suggested validation
`@flow_id.setter`
def flow_id(self, value: str) -> None:
- self._short_id = value
+ self._short_id = Element._valid_id(value)
`@id.setter`
def id(self, value: str) -> None:
- self._short_id = value
+ self._short_id = Element._valid_id(value)📝 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.
| @property | |
| def label_full(self) -> str: | |
| return f'{self.component}({self.label})' | |
| def flow_id(self) -> str: | |
| """The short flow identifier (e.g. ``'Heat'``). | |
| This is the user-facing name. Defaults to the bus name if not set explicitly. | |
| """ | |
| return self._short_id | |
| @flow_id.setter | |
| def flow_id(self, value: str) -> None: | |
| self._short_id = value | |
| @property | |
| def id(self) -> str: | |
| """The qualified identifier: ``component(flow_id)``.""" | |
| return f'{self.component}({self._short_id})' | |
| @id.setter | |
| def id(self, value: str) -> None: | |
| self._short_id = value | |
| `@property` | |
| def flow_id(self) -> str: | |
| """The short flow identifier (e.g. ``'Heat'``). | |
| This is the user-facing name. Defaults to the bus name if not set explicitly. | |
| """ | |
| return self._short_id | |
| `@flow_id.setter` | |
| def flow_id(self, value: str) -> None: | |
| self._short_id = Element._valid_id(value) | |
| `@property` | |
| def id(self) -> str: | |
| """The qualified identifier: ``component(flow_id)``.""" | |
| return f'{self.component}({self._short_id})' | |
| `@id.setter` | |
| def id(self, value: str) -> None: | |
| self._short_id = Element._valid_id(value) |
🤖 Prompt for AI Agents
In `@flixopt/elements.py` around lines 806 - 826, The setters for flow_id and id
currently assign directly to self._short_id and bypass _valid_id; update the
flow_id.setter and id.setter to validate the incoming value by calling
self._valid_id(value) (or the appropriate validation helper) and raise a
ValueError with a clear message if validation fails, otherwise assign to
self._short_id; ensure id.setter still preserves the intended format (accepting
the same short identifier string used elsewhere) and reuse the same validation
logic as flow_id.setter so the ID invariants are enforced post-init.
| scenario_independent_sizes: Controls whether investment sizes are equalized across scenarios. | ||
| - True: All sizes are shared/equalized across scenarios | ||
| - False: All sizes are optimized separately per scenario | ||
| - list[str]: Only specified components (by label_full) are equalized across scenarios | ||
| - list[str]: Only specified components (by id) are equalized across scenarios | ||
| scenario_independent_flow_rates: Controls whether flow rates are equalized across scenarios. | ||
| - True: All flow rates are shared/equalized across scenarios | ||
| - False: All flow rates are optimized separately per scenario | ||
| - list[str]: Only specified flows (by label_full) are equalized across scenarios | ||
| - list[str]: Only specified flows (by id) are equalized across scenarios | ||
|
|
There was a problem hiding this comment.
Docs/type hints: clarify what goes into scenario_independent_sizes lists (component ids vs flow ids).
The docstring currently says “components (by id)” for sizes, but size equalization typically targets investable flows/storages. If lists are expected to contain qualified ids like Component(flow_id), updating the wording would prevent user confusion.
Also applies to: 2134-2141, 2159-2166
🤖 Prompt for AI Agents
In `@flixopt/flow_system.py` around lines 232 - 240, The docstrings for
scenario_independent_sizes and scenario_independent_flow_rates are ambiguous
about what IDs the lists should contain; update the descriptions and type hints
so they explicitly state that lists must contain investable flow/storage IDs
(not generic component names) — e.g., qualified component identifiers such as
"Component(flow_id)" or the flow/storage id string — and clarify that
scenario_independent_sizes targets investable flows/storages (sizes/investments)
whereas scenario_independent_flow_rates targets flow rate IDs; apply this
wording update to every docstring mentioning these params (including the other
occurrences noted) and include a short example of the expected string format.
flixopt/id_list.py
Outdated
| # Short-key fallback | ||
| if self._short_key_fn is not None: | ||
| for elem in self._data.values(): | ||
| if self._short_key_fn(elem) == key: | ||
| return elem |
There was a problem hiding this comment.
Short-key lookup is ambiguous when duplicates exist
Line 104-108 returns the first match for a short key. If short keys are not unique (e.g., flows with the same flow_id across components), lookups become nondeterministic and can return the wrong element. Consider detecting duplicates and raising a clear error that lists the matching full ids.
🛠️ Suggested fix (detect and report ambiguity)
- if self._short_key_fn is not None:
- for elem in self._data.values():
- if self._short_key_fn(elem) == key:
- return elem
+ if self._short_key_fn is not None:
+ matches = [elem for elem in self._data.values() if self._short_key_fn(elem) == key]
+ if len(matches) == 1:
+ return matches[0]
+ if len(matches) > 1:
+ match_ids = [self._key_fn(e) for e in matches]
+ raise KeyError(
+ f'"{key}" is ambiguous in {self._display_name}. '
+ f'Matches: {", ".join(match_ids)}'
+ )📝 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.
| # Short-key fallback | |
| if self._short_key_fn is not None: | |
| for elem in self._data.values(): | |
| if self._short_key_fn(elem) == key: | |
| return elem | |
| # Short-key fallback | |
| if self._short_key_fn is not None: | |
| matches = [elem for elem in self._data.values() if self._short_key_fn(elem) == key] | |
| if len(matches) == 1: | |
| return matches[0] | |
| if len(matches) > 1: | |
| match_ids = [self._key_fn(e) for e in matches] | |
| raise KeyError( | |
| f'"{key}" is ambiguous in {self._display_name}. ' | |
| f'Matches: {", ".join(match_ids)}' | |
| ) |
🤖 Prompt for AI Agents
In `@flixopt/id_list.py` around lines 104 - 108, The current short-key fallback
loop using self._short_key_fn over self._data.values() returns the first match
and is nondeterministic when multiple elements share the same short key; change
the logic in the lookup (where _short_key_fn and _data are used) to collect all
elements whose _short_key_fn(elem) == key, and if more than one match is found
raise a clear ValueError (or a specific exception) that includes the full
identifiers of the matching elements so callers can resolve the ambiguity
instead of silently returning the first match.
| obj: The object to create repr for | ||
| excluded_params: Set of parameter names to exclude (e.g., {'self', 'inputs', 'outputs'}) | ||
| Default excludes 'self', 'label', and 'kwargs' | ||
| label_as_positional: If True and 'label' param exists, show it as first positional arg | ||
| Default excludes 'self', 'id', and 'kwargs' | ||
| id_as_positional: If True and 'id' param exists, show it as first positional arg | ||
| skip_default_size: Deprecated. Previously skipped size=CONFIG.Modeling.big, now size=None is default. | ||
| label_as_positional: Deprecated alias for id_as_positional. | ||
|
|
||
| Returns: | ||
| Formatted repr string like: ClassName("label", param=value) | ||
| Formatted repr string like: ClassName("id", param=value) | ||
| """ | ||
| if label_as_positional is not None: | ||
| id_as_positional = label_as_positional | ||
|
|
||
| if excluded_params is None: | ||
| excluded_params = {'self', 'label', 'kwargs'} | ||
| excluded_params = {'self', 'id', 'label', 'kwargs'} | ||
| else: | ||
| # Always exclude 'self' | ||
| excluded_params = excluded_params | {'self'} | ||
| # Always exclude 'self' and deprecated 'label' | ||
| excluded_params = excluded_params | {'self', 'label'} | ||
|
|
There was a problem hiding this comment.
Docstring default exclusions omit label
The docstring (Line 1239-1242) says defaults exclude self, id, and kwargs, but the code always excludes label too. Please update the docstring to match behavior.
✏️ Suggested doc update
- excluded_params: Set of parameter names to exclude (e.g., {'self', 'inputs', 'outputs'})
- Default excludes 'self', 'id', and 'kwargs'
+ excluded_params: Set of parameter names to exclude (e.g., {'self', 'inputs', 'outputs'})
+ Default excludes 'self', 'id', 'label', and 'kwargs'📝 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.
| obj: The object to create repr for | |
| excluded_params: Set of parameter names to exclude (e.g., {'self', 'inputs', 'outputs'}) | |
| Default excludes 'self', 'label', and 'kwargs' | |
| label_as_positional: If True and 'label' param exists, show it as first positional arg | |
| Default excludes 'self', 'id', and 'kwargs' | |
| id_as_positional: If True and 'id' param exists, show it as first positional arg | |
| skip_default_size: Deprecated. Previously skipped size=CONFIG.Modeling.big, now size=None is default. | |
| label_as_positional: Deprecated alias for id_as_positional. | |
| Returns: | |
| Formatted repr string like: ClassName("label", param=value) | |
| Formatted repr string like: ClassName("id", param=value) | |
| """ | |
| if label_as_positional is not None: | |
| id_as_positional = label_as_positional | |
| if excluded_params is None: | |
| excluded_params = {'self', 'label', 'kwargs'} | |
| excluded_params = {'self', 'id', 'label', 'kwargs'} | |
| else: | |
| # Always exclude 'self' | |
| excluded_params = excluded_params | {'self'} | |
| # Always exclude 'self' and deprecated 'label' | |
| excluded_params = excluded_params | {'self', 'label'} | |
| obj: The object to create repr for | |
| excluded_params: Set of parameter names to exclude (e.g., {'self', 'inputs', 'outputs'}) | |
| Default excludes 'self', 'id', 'label', and 'kwargs' | |
| id_as_positional: If True and 'id' param exists, show it as first positional arg | |
| skip_default_size: Deprecated. Previously skipped size=CONFIG.Modeling.big, now size=None is default. | |
| label_as_positional: Deprecated alias for id_as_positional. | |
| Returns: | |
| Formatted repr string like: ClassName("id", param=value) | |
| """ | |
| if label_as_positional is not None: | |
| id_as_positional = label_as_positional | |
| if excluded_params is None: | |
| excluded_params = {'self', 'id', 'label', 'kwargs'} | |
| else: | |
| # Always exclude 'self' and deprecated 'label' | |
| excluded_params = excluded_params | {'self', 'label'} |
🤖 Prompt for AI Agents
In `@flixopt/io.py` around lines 1239 - 1257, The docstring for the repr helper is
inaccurate: it says the default excluded params are {'self', 'id', 'kwargs'} but
the implementation of the function (parameters excluded_params,
label_as_positional, id_as_positional) actually always excludes 'label' as well;
update the docstring to list the true defaults (e.g., 'self', 'id', 'label', and
'kwargs') and note that 'label' is a deprecated alias for 'id' or that
label_as_positional is a deprecated alias for id_as_positional so the
documentation matches the behavior of the code paths that union excluded_params
with {'self','label'}.
flixopt/results.py
Outdated
| components: Dictionary mapping component ids to ComponentResults objects | ||
| buses: Dictionary mapping bus ids to BusResults objects | ||
| effects: Dictionary mapping effect ids to EffectResults objects | ||
| timesteps_extra: Extended time index including boundary conditions |
There was a problem hiding this comment.
Docstring still says “Dictionary” even though these are IdLists.
This can mislead users about available behaviors (e.g., ordering, short-key fallback). Consider updating to “IdList” or “mapping-like IdList.”
Also applies to: 159-160
🤖 Prompt for AI Agents
In `@flixopt/results.py` around lines 100 - 103, Update the docstring entries that
currently say "Dictionary" to instead state "IdList" (or "mapping-like IdList"
where appropriate) for the fields components, buses, effects, and
timesteps_extra so the documentation reflects the actual type/behavior
(ordering, short-key fallback). Also apply the same replacement for the other
occurrences mentioned around the later docstring block (the entries at the
159-160 area). Reference the field names components, buses, effects,
timesteps_extra and make the wording consistent across both docstring locations.
| @property | ||
| def id(self) -> str: | ||
| """The unique identifier of this element. | ||
|
|
||
| For most elements this is the name passed to the constructor. | ||
| For flows this returns the qualified form: ``component(short_id)``. | ||
| """ | ||
| return self._short_id | ||
|
|
||
| @id.setter | ||
| def id(self, value: str) -> None: | ||
| self._short_id = value | ||
|
|
There was a problem hiding this comment.
Enforce _valid_id in Element.id setter.
The setter bypasses validation, so invalid characters can be introduced after init and break naming/serialization.
🔧 Suggested validation
`@id.setter`
def id(self, value: str) -> None:
- self._short_id = value
+ self._short_id = Element._valid_id(value)🤖 Prompt for AI Agents
In `@flixopt/structure.py` around lines 2031 - 2043, The id setter currently
assigns to _short_id without validation; update the Element.id setter to call
the existing _valid_id (or validation helper) on the incoming value and raise a
ValueError (or appropriate exception) if it fails, otherwise set self._short_id
to the validated value so invalid characters cannot be introduced after
initialization (refer to the Element.id property/setter and the _valid_id
validation function).
Split DEPRECATION_REMOVAL_VERSION into V7 (v6.x deprecations) and V8 (v7.x label→id deprecations) so the label/Flow constructor compat layer isn't removed in the same release it was introduced. Also fixes the penalty-objective guard to compare self.id, adds _valid_id to all id setters, and raises on ambiguous short-key lookups in IdList. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Renames
labeltoidacross the entire API, introducesIdList[T]as the unified container class, and redesigns theFlowconstructor — all with full backward compatibility via deprecation bridges.Breaking Changes
label→id: All element constructors useidinstead oflabel. Oldlabel=kwarg and.label/.label_fullproperties still work withDeprecationWarning(removal in v7.0.0).Flowconstructor:busis now the first positional arg;iddefaults to the bus name. OldFlow(label, bus)form triggers deprecation warning.Flow.idreturns qualified name: e.g.,Boiler(Q_fu)instead ofQ_fu. Useflow._short_idfor the raw name.FlowContainer,ElementContainer,ResultsContainer→IdList[T].EffectCollectionandCarrierContainernow inherit fromIdList.Key Changes
IdList[T](flixopt/id_list.py): Generic frozen ordered container with primary key + short-key fallback + positional index access. Helpful error messages withdifflib.get_close_matchessuggestions.Element(label=...)routes through_handle_deprecated_kwargtoid. Allidparams made optional (str | None = None) solabelkwarg can flow through..ncfiles withlabelkeys auto-convert toidon load. Serialization uses_short_idto avoid storing qualified Flow names._short_id(notFlow.id) for consistency withsigned_coefficientsflow_map.Files Changed (20 files, +1110 / -690)
flixopt/id_list.pyIdList[T]class + factory helpersflixopt/structure.pyElement.id, deprecated.label/.label_full, serialization fixesflixopt/elements.pyFlowconstructor redesign,Component/BususeIdListflixopt/components.pylabel→idflixopt/effects.pyEffect+EffectCollection(IdList)flixopt/linear_converters.pylabel→id,_short_idfor conversion factorsflixopt/flow_system.pyelement_id_list()flixopt/batched.py*Dataclasses:.label/.label_full→.idflixopt/results.pyResultsContainer→IdListflixopt/carrier.pyCarrierContainer(IdList)flixopt/io.pybuild_repr_from_initupdates.label_full→.idacross 8 filesType of Change
Testing
dash_cytoscapedep — pre-existing)label→idconversionChecklist
Summary by CodeRabbit
Release Notes
New Features
IdList, a new generic container for named elements with dict-like access and enhanced error messagesFlow.flow_idproperty for explicit flow identifiersBreaking Changes
labeltoidacross component and element constructors; useid=...instead oflabel=...Flowconstructor: bus identifier is now the first positional argument, with optionalflow_idparameterIdListDeprecations
labelandlabel_fullparameter usage supported with warnings; migrate toidandid-based access patterns