Feature/containers-and-reprs-for-better-element-organization #443
Conversation
WalkthroughAdds a container subsystem and applies it across elements, results, effects, and FlowSystem; introduces Component._connect_flows() to validate and attach flows during initialization, many new repr and io/formatting helpers, updates tests to dict‑like element access (flow_system['label']), and deprecates all_elements. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant FS as FlowSystem
participant C as Component
participant F as Flow
Note over FS: containers: components, buses, flows
User->>FS: create FlowSystem(...)
FS->>FS: init ElementContainer('components','buses',...)
User->>C: instantiate Component(label, inputs, outputs)
C->>C: _check_unique_flow_labels()
C->>C: _connect_flows()
loop each flow in inputs/outputs
C->>F: validate flow.component (UnknownComponent or self)
alt foreign component attached
F-->>C: raise ValueError (conflict)
else
C->>F: set flow.component and is_input_in_component flag
end
end
User->>FS: access element via FS['label']
FS->>FS: lookup in container groups (_get_container_groups)
FS-->>User: return element
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
flixopt/commons.py (1)
22-22: Public re‑exports look good; consider symmetry with CompositeContainerMixin.To keep the public API cohesive, also re‑export CompositeContainerMixin so downstream modules can consistently import all container types from commons.
Apply:
-from .structure import ContainerMixin, ElementContainer, ResultsContainer +from .structure import CompositeContainerMixin, ContainerMixin, ElementContainer, ResultsContainer @@ 'FlowSystem', - 'ContainerMixin', + 'CompositeContainerMixin', + 'ContainerMixin', 'ElementContainer', 'ResultsContainer',Also applies to: 38-40
flixopt/structure.py (3)
901-919: Make containers properly generic; stabilize repr ordering.Current classes use TypeVar T but don’t inherit Generic[T]. Also, printing keys in insertion order may vary; sort for deterministic repr.
-from typing import ( +from typing import ( TYPE_CHECKING, Any, - Generic, + Generic, Literal, TypeVar, ) @@ -class ContainerMixin(dict[str, T]): +class ContainerMixin(dict[str, T], Generic[T]): @@ - for name in self.keys(): + for name in sorted(self.keys()): r += f' * {name}\n'Apply the same Generic[T] to ElementContainer and ResultsContainer:
-class ElementContainer(ContainerMixin[T]): +class ElementContainer(ContainerMixin[T], Generic[T]): @@ -class ResultsContainer(ContainerMixin[T]): +class ResultsContainer(ContainerMixin[T], Generic[T]):
1118-1124: Avoid re‑import inside method.get_close_matches is already imported at module scope. Drop the inner import to reduce overhead and keep consistency.
- # Element not found - provide helpful error - from difflib import get_close_matches + # Element not found - provide helpful error
1155-1162: Avoid O(n²) in values()/items(); iterate containers directly.Current implementation resolves each key via getitem repeatedly. Iterate group containers to collect values and pairs in one pass.
- def values(self): - """Return all element objects across all containers.""" - return [self[key] for key in self] + def values(self): + """Return all element objects across all containers.""" + vals = [] + for container in self._get_container_groups().values(): + vals.extend(container.values()) + return vals @@ - def items(self): - """Return (label, element) pairs for all elements.""" - return [(key, self[key]) for key in self] + def items(self): + """Return (label, element) pairs for all elements.""" + items = [] + for container in self._get_container_groups().values(): + items.extend(container.items()) + return itemsflixopt/flow_system.py (3)
713-733: Unify access precedence and avoid duplication with CompositeContainerMixin.getitem/keys/values/items here duplicate mixin behavior and use a different precedence (all_elements merges in a different order than _get_container_groups). This can cause subtle inconsistencies.
- def __getitem__(self, item) -> Element: - """Get element by exact label with helpful error messages.""" - if item in self.all_elements: - return self.all_elements[item] - ... - raise KeyError(error_msg) + # Remove this override and rely on CompositeContainerMixin.__getitem__() + # to keep lookup precedence consistent with _get_container_groups().If you prefer to keep these methods, align all_elements to the same order as _get_container_groups:
@property def all_elements(self) -> dict[str, Element]: - return {**self.components, **self.effects.effects, **self.flows, **self.buses} + # Match group order: Components, Buses, Effects, Flows + return {**self.components, **self.buses, **self.effects.effects, **self.flows}Please confirm which approach you prefer; I can provide a follow-up patch removing the overrides throughout.
743-758: Avoid duplicating keys/values/items; delegate to mixin.To reduce maintenance and ensure consistent ordering, drop these overrides and use CompositeContainerMixin.keys/values/items.
- def __len__(self) -> int: - """Return total count of all elements.""" - return len(self.all_elements) - - def keys(self): - """Return all element labels.""" - return list(self.all_elements.keys()) - - def values(self): - """Return all element objects.""" - return list(self.all_elements.values()) - - def items(self): - """Return (label, element) pairs for all elements.""" - return list(self.all_elements.items()) + # Rely on CompositeContainerMixin for __len__, keys, values, items
769-773: flows property recomputes container each call; consider lightweight caching.If accessed frequently, cache the ElementContainer and invalidate on add_elements(). Optional.
- def flows(self) -> ElementContainer[Flow]: + def flows(self) -> ElementContainer[Flow]: + # Optional memoization: + # if not hasattr(self, '_flows_cache'): self._flows_cache = ... set_of_flows = {flow for comp in self.components.values() for flow in comp.inputs + comp.outputs} flows_dict = {flow.label_full: flow for flow in set_of_flows} return ElementContainer(elements=flows_dict, element_type_name='flows')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
flixopt/commons.py(2 hunks)flixopt/elements.py(2 hunks)flixopt/flow_system.py(7 hunks)flixopt/results.py(4 hunks)flixopt/structure.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
flixopt/commons.py (1)
flixopt/structure.py (3)
ContainerMixin(905-1006)ElementContainer(1009-1018)ResultsContainer(1021-1030)
flixopt/structure.py (3)
flixopt/flow_system.py (5)
values(751-753)keys(747-749)_get_container_groups(759-766)all_elements(775-776)items(755-757)flixopt/elements.py (1)
label_full(493-494)flixopt/results.py (1)
_get_container_groups(285-292)
flixopt/results.py (2)
flixopt/structure.py (9)
CompositeContainerMixin(1033-1206)ResultsContainer(1021-1030)solution(158-181)items(1159-1161)items(1437-1438)get(1285-1290)get(1450-1452)_get_container_groups(1079-1097)_format_grouped_containers(1163-1206)flixopt/flow_system.py (3)
items(755-757)flows(769-772)_get_container_groups(759-766)
flixopt/flow_system.py (1)
flixopt/structure.py (16)
CompositeContainerMixin(1033-1206)Element(857-898)ElementContainer(1009-1018)FlowSystemModel(84-255)add(945-953)add(1446-1448)label_full(879-880)label_full(1323-1324)_format_grouped_containers(1163-1206)keys(1151-1153)keys(1440-1441)values(1155-1157)values(1443-1444)items(1159-1161)items(1437-1438)_get_container_groups(1079-1097)
flixopt/elements.py (1)
flixopt/structure.py (2)
label_full(879-880)label_full(1323-1324)
🔇 Additional comments (7)
flixopt/results.py (3)
20-21: Import of CompositeContainerMixin is appropriate.
57-59: Adopting CompositeContainerMixin improves UX.
295-297: Grouped repr is a nice usability win.flixopt/flow_system.py (4)
35-36: New imports align with containerized architecture.
107-109: Using ElementContainer for components/buses is clean and adds validation.
662-694: Readable repr; helpful system overview.
759-767: Container grouping is clear and ordered.
…nected to another Component
# Conflicts: # tests/test_examples.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
flixopt/effects.py (1)
590-592: Reverting iterator to yield keys fixes dict-like behavior.Yielding keys restores correct dict(self.effects)/CompositeContainerMixin behavior. LGTM.
🧹 Nitpick comments (3)
flixopt/flow_system.py (1)
728-735: Simplify flows container construction (avoid dict detour).Passing a dict then re-deriving keys is unnecessary; a list suffices.
- self._flows_cache = ElementContainer({f.label_full: f for f in flows}, element_type_name='flows') + self._flows_cache = ElementContainer(flows, element_type_name='flows')flixopt/elements.py (1)
564-587: Remove or integrate unused helper_format_sizeinFlowclass.Verification confirms
_format_size(lines 564–587 inflixopt/elements.py) has no call sites anywhere in the codebase. This method is dead code and should either be removed or integrated into the repr path if it serves a purpose.flixopt/results.py (1)
243-244: Consider moving ResultsContainer import to module level for consistency.The local import of
ResultsContainerinside__init__is inconsistent with the top-level import ofCompositeContainerMixinat line 20. Since both classes are from the same.structuremodule, this local import pattern appears unnecessary unless there's a specific circular import issue.Apply this diff to move the import to the top level:
from . import io as fx_io from . import plotting from .color_processing import process_colors from .config import CONFIG from .flow_system import FlowSystem -from .structure import CompositeContainerMixin +from .structure import CompositeContainerMixin, ResultsContainerThen remove the local import:
self.folder = pathlib.Path(folder) if folder is not None else pathlib.Path.cwd() / 'results' # Create ResultsContainers for better access patterns - from .structure import ResultsContainer - components_dict = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
flixopt/components.py(2 hunks)flixopt/effects.py(10 hunks)flixopt/elements.py(5 hunks)flixopt/flow_system.py(12 hunks)flixopt/io.py(3 hunks)flixopt/results.py(12 hunks)flixopt/structure.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flixopt/components.py
🧰 Additional context used
🧬 Code graph analysis (6)
flixopt/effects.py (2)
flixopt/structure.py (16)
Element(823-878)ElementContainer(999-1008)ElementModel(1445-1466)FlowSystemModel(85-254)Submodel(1204-1378)_format_repr(848-856)add(934-942)add(1436-1438)keys(1148-1150)keys(1430-1431)label_full(845-846)label_full(1318-1319)items(1159-1164)items(1427-1428)values(1152-1157)values(1433-1434)flixopt/io.py (1)
build_metadata_info(853-868)
flixopt/structure.py (4)
flixopt/results.py (2)
flow_system(341-357)_get_container_groups(287-294)flixopt/elements.py (1)
label_full(551-552)flixopt/io.py (3)
format_sections_with_headers(836-850)build_repr_from_init(619-794)format_title_with_underline(823-833)flixopt/flow_system.py (1)
_get_container_groups(718-725)
flixopt/elements.py (3)
flixopt/structure.py (5)
label_full(845-846)label_full(1318-1319)add(934-942)add(1436-1438)_format_repr(848-856)flixopt/io.py (4)
build_metadata_info(853-868)build_repr_from_init(619-794)format_flow_details(797-820)numeric_to_str_for_repr(554-616)flixopt/interface.py (2)
InvestParameters(672-1084)format_for_repr(1054-1073)
flixopt/io.py (3)
flixopt/structure.py (4)
values(1152-1157)values(1433-1434)label_full(845-846)label_full(1318-1319)flixopt/config.py (2)
CONFIG(70-377)Modeling(184-195)flixopt/elements.py (1)
label_full(551-552)
flixopt/results.py (3)
flixopt/structure.py (11)
CompositeContainerMixin(1026-1201)ResultsContainer(1011-1020)items(1159-1164)items(1427-1428)get(1280-1285)get(1440-1442)_get_container_groups(1078-1096)_format_grouped_containers(1166-1201)values(1152-1157)values(1433-1434)_format_repr(848-856)flixopt/flow_system.py (2)
flows(728-734)_get_container_groups(718-725)flixopt/io.py (1)
format_title_with_underline(823-833)
flixopt/flow_system.py (3)
flixopt/structure.py (12)
CompositeContainerMixin(1026-1201)Element(823-878)ElementContainer(999-1008)FlowSystemModel(85-254)values(1152-1157)values(1433-1434)label_full(845-846)label_full(1318-1319)add(934-942)add(1436-1438)_format_grouped_containers(1166-1201)_get_container_groups(1078-1096)flixopt/effects.py (1)
EffectCollection(481-652)flixopt/io.py (1)
format_title_with_underline(823-833)
⏰ 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). (4)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (7)
flixopt/structure.py (1)
848-857: Standardized repr helper for Elements looks good.Consistent constructor-style repr with optional info and hidden default size is a solid improvement.
flixopt/results.py (6)
246-270: Excellent container refactoring with proper backward compatibility.The wrapping of components, buses, effects, and flows in
ResultsContainerinstances provides a cleaner architecture while maintaining dict-like access patterns. The_has_flow_dataflag correctly tracks flow data availability for older result files, and empty containers are properly initialized when flow data is absent.
287-294: LGTM: Clean implementation of container groups.The
_get_container_groups()method correctly implements the required interface fromCompositeContainerMixin, returning an ordered dictionary of all result containers for unified access.
296-302: LGTM: Enhanced representation with grouped containers.The new
__repr__()method provides clear, grouped visualization of all results using the mixin's formatting helper. This improves discoverability and debugging experience.
567-568: Excellent: Flow data guards successfully address past review concerns.The
_has_flow_dataguards inflow_rates()andsizes()methods preventxr.concat([])crashes when working with older result files (pre-v2.2.0) that lack flow data. The error messages clearly explain the limitation.Based on learnings
Also applies to: 630-631
1129-1143: LGTM: Consistent repr formatting pattern established.The
_format_repr()helper provides a clean, reusable pattern for formatting element result representations with class name, label, optional metadata, and dataset preview.
1636-1639: LGTM: Well-structured repr implementations across result classes.All result classes now provide contextual
__repr__()implementations:
_NodeResults: Shows flow counts and input/output breakdownComponentResults: Adds storage indicationEffectResults: Shows contributor countFlowResults: Displays connection (start → end)This consistent pattern enhances the developer experience and debugging capabilities.
Also applies to: 1918-1922, 1939-1944, 1982-1985
flixopt/effects.py
Outdated
| constraint_types = [] | ||
| if hasattr(self, 'maximum_per_hour') and self.maximum_per_hour is not None: | ||
| constraint_types.append('per_hour') | ||
| if hasattr(self, 'maximum_temporal') and self.maximum_temporal is not None: | ||
| constraint_types.append('temporal') | ||
| if hasattr(self, 'maximum_periodic') and self.maximum_periodic is not None: | ||
| constraint_types.append('periodic') | ||
| if hasattr(self, 'maximum_total') and self.maximum_total is not None: | ||
| constraint_types.append('total') | ||
|
|
||
| if constraint_types: | ||
| parts.append('constraints: ' + '+'.join(constraint_types)) | ||
|
|
||
| info = fx_io.build_metadata_info(parts) |
There was a problem hiding this comment.
Repr constraint flags miss min-only bounds.
Effect.repr only adds flags when maximum_* is set; min-only constraints won’t be shown. Include both min and max checks.
- # Constraint types
- constraint_types = []
- if hasattr(self, 'maximum_per_hour') and self.maximum_per_hour is not None:
- constraint_types.append('per_hour')
- if hasattr(self, 'maximum_temporal') and self.maximum_temporal is not None:
- constraint_types.append('temporal')
- if hasattr(self, 'maximum_periodic') and self.maximum_periodic is not None:
- constraint_types.append('periodic')
- if hasattr(self, 'maximum_total') and self.maximum_total is not None:
- constraint_types.append('total')
+ # Constraint types (flag if either min or max bound is set)
+ constraint_types = []
+ if any(getattr(self, n, None) is not None for n in ('minimum_per_hour', 'maximum_per_hour')):
+ constraint_types.append('per_hour')
+ if any(getattr(self, n, None) is not None for n in ('minimum_temporal', 'maximum_temporal')):
+ constraint_types.append('temporal')
+ if any(getattr(self, n, None) is not None for n in ('minimum_periodic', 'maximum_periodic')):
+ constraint_types.append('periodic')
+ if any(getattr(self, n, None) is not None for n in ('minimum_total', 'maximum_total')):
+ constraint_types.append('total')📝 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.
| constraint_types = [] | |
| if hasattr(self, 'maximum_per_hour') and self.maximum_per_hour is not None: | |
| constraint_types.append('per_hour') | |
| if hasattr(self, 'maximum_temporal') and self.maximum_temporal is not None: | |
| constraint_types.append('temporal') | |
| if hasattr(self, 'maximum_periodic') and self.maximum_periodic is not None: | |
| constraint_types.append('periodic') | |
| if hasattr(self, 'maximum_total') and self.maximum_total is not None: | |
| constraint_types.append('total') | |
| if constraint_types: | |
| parts.append('constraints: ' + '+'.join(constraint_types)) | |
| info = fx_io.build_metadata_info(parts) | |
| # Constraint types (flag if either min or max bound is set) | |
| constraint_types = [] | |
| if any(getattr(self, n, None) is not None for n in ('minimum_per_hour', 'maximum_per_hour')): | |
| constraint_types.append('per_hour') | |
| if any(getattr(self, n, None) is not None for n in ('minimum_temporal', 'maximum_temporal')): | |
| constraint_types.append('temporal') | |
| if any(getattr(self, n, None) is not None for n in ('minimum_periodic', 'maximum_periodic')): | |
| constraint_types.append('periodic') | |
| if any(getattr(self, n, None) is not None for n in ('minimum_total', 'maximum_total')): | |
| constraint_types.append('total') | |
| if constraint_types: | |
| parts.append('constraints: ' + '+'.join(constraint_types)) | |
| info = fx_io.build_metadata_info(parts) |
🤖 Prompt for AI Agents
In flixopt/effects.py around lines 403 to 416, the repr builder only adds
constraint type flags when maximum_* attributes are set, so min-only constraints
are omitted; update the logic to check both minimum_* and maximum_* (presence
and not None) for each constraint category (per_hour, temporal, periodic, total)
and append the corresponding flag if either bound exists, then continue to build
parts and call fx_io.build_metadata_info as before.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
flixopt/io.py (1)
629-808: LGTM! Comprehensive repr builder with solid introspection logic.The function handles a wide range of scenarios:
- Introspects init signatures to show only non-default parameters
- Handles special cases (empty containers, arrays, CONFIG.Modeling.big)
- Formats complex types (dicts with numeric values, arrays) readably
- Provides fallback behavior on errors
The 180-line length is acceptable given the comprehensive coverage, though you could consider extracting value formatting (lines 744-782) into a separate helper method for improved testability.
Optional refactor: Consider extracting lines 744-782 (value formatting logic) into a
_format_value_for_repr()helper method to improve testability and readability:def _format_value_for_repr(self, value) -> str: """Format a single value for display in repr.""" if isinstance(value, (int, float, np.integer, np.floating, np.ndarray, pd.Series, pd.DataFrame, xr.DataArray)): try: return numeric_to_str_for_repr(value) except Exception: # ... truncation logic elif isinstance(value, dict): # ... dict formatting logic else: # ... default repr logicThis would reduce the cognitive complexity of build_repr_from_init while maintaining functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
flixopt/components.py(2 hunks)flixopt/effects.py(9 hunks)flixopt/elements.py(5 hunks)flixopt/io.py(3 hunks)flixopt/results.py(8 hunks)flixopt/structure.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flixopt/components.py
🧰 Additional context used
🧬 Code graph analysis (5)
flixopt/effects.py (1)
flixopt/structure.py (15)
Element(823-868)ElementContainer(989-998)ElementModel(1435-1456)FlowSystemModel(85-254)Submodel(1194-1368)add(924-932)add(1426-1428)keys(1138-1140)keys(1420-1421)label_full(845-846)label_full(1308-1309)items(1149-1154)items(1417-1418)values(1142-1147)values(1423-1424)
flixopt/elements.py (3)
flixopt/structure.py (2)
label_full(845-846)label_full(1308-1309)flixopt/io.py (3)
build_repr_from_init(629-807)format_flow_details(810-833)numeric_to_str_for_repr(554-626)flixopt/interface.py (2)
InvestParameters(672-1084)format_for_repr(1054-1073)
flixopt/structure.py (4)
flixopt/results.py (2)
flow_system(341-357)_get_container_groups(287-294)flixopt/elements.py (1)
label_full(537-538)flixopt/io.py (3)
format_sections_with_headers(849-863)build_repr_from_init(629-807)format_title_with_underline(836-846)flixopt/flow_system.py (1)
_get_container_groups(718-725)
flixopt/results.py (3)
flixopt/structure.py (10)
CompositeContainerMixin(1016-1191)ResultsContainer(1001-1010)items(1149-1154)items(1417-1418)get(1270-1275)get(1430-1432)_get_container_groups(1068-1086)_format_grouped_containers(1156-1191)values(1142-1147)values(1423-1424)flixopt/flow_system.py (2)
flows(728-734)_get_container_groups(718-725)flixopt/io.py (1)
format_title_with_underline(836-846)
flixopt/io.py (4)
flixopt/structure.py (6)
values(1142-1147)values(1423-1424)items(1149-1154)items(1417-1418)label_full(845-846)label_full(1308-1309)flixopt/interface.py (1)
items(444-451)flixopt/config.py (1)
Modeling(184-195)flixopt/elements.py (1)
label_full(537-538)
🔇 Additional comments (19)
flixopt/elements.py (4)
121-156: LGTM! Flow attachment and validation logic is well-structured.The _connect_flows() method properly:
- Validates flows are unattached or belong to the current component
- Sets ownership and direction flags
- Validates prevent_simultaneous_flows contains only local flows
- Deduplicates while preserving order using object identity
The implementation addresses the concerns raised in previous reviews.
157-161: LGTM! Clean repr implementation using centralized utilities.The repr method leverages fx_io utilities for consistent formatting across the codebase and properly excludes inputs/outputs from the parameter list since they're shown separately via format_flow_details().
264-266: LGTM! Consistent repr pattern.The Bus repr follows the same pattern as Component, properly delegating to the parent class and appending flow details.
545-567: LGTM! Format helpers are well-structured.The inline imports (lines 547-550) are appropriate here—they avoid circular dependencies and the performance impact is negligible since these are only called during repr operations.
The logic correctly hides the default CONFIG.Modeling.big value and delegates InvestParameters formatting to the parameter class itself.
flixopt/io.py (2)
554-627: LGTM! Robust numeric formatting with comprehensive edge-case handling.The function properly:
- Handles various numeric types (scalars, arrays, DataFrames, Series)
- Filters non-finite values before computing statistics (lines 597-605)
- Distinguishes between truly uniform values, display-uniform values (prefix
~), and ranges- Returns clear fallback values for empty or all-NaN inputs
This addresses the NaN/Inf robustness concerns from previous reviews.
810-881: LGTM! Clean formatting utilities with clear single responsibilities.These helper functions are well-designed:
format_flow_details(): Cleanly handles optional inputs/outputs with bullet-point formattingformat_title_with_underline(): Simple, reusable title formattingformat_sections_with_headers(): Consistent section renderingbuild_metadata_info(): Flexible metadata string building with filteringEach has a clear, focused purpose and good documentation.
flixopt/structure.py (4)
871-877: LGTM! Efficient natural sorting for numeric labels.The precompiled regex pattern and sort key function enable intuitive ordering of labels like "bus1, bus2, bus10" instead of the lexicographic "bus1, bus10, bus2". Good UX enhancement.
884-987: LGTM! Excellent container base class with superior error handling.The ContainerMixin provides a solid foundation:
- Validation: add() and setitem prevent duplicates and label mismatches (lines 924-942)
- Error UX: getitem uses
get_close_matches()to suggest similar labels when lookups fail (lines 944-971)—this is a significant usability improvement- Formatting: Natural sorting in repr (line 983) ensures readable output
The abstract _get_label() method (lines 912-922) properly delegates label extraction to subclasses.
989-1011: LGTM! Clean container specializations with appropriate label extraction.Both containers properly specialize ContainerMixin:
- ElementContainer uses
label_fullfor elements that may have composite labels- ResultsContainer uses
labelfor results objectsThe type hints (ContainerMixin[T]) enable proper IDE autocomplete and type checking.
1016-1192: LGTM! Well-designed composite container with excellent documentation.The CompositeContainerMixin provides unified access across multiple typed containers:
- Type safety: Generic[T_element] enables proper IDE support for union types (line 1016)
- Complete dict interface: All standard methods implemented (lines 1088-1154)
- Error handling: Cross-container search with helpful suggestions on KeyError (lines 1088-1123)
- Documentation: Comprehensive docstring with concrete examples (lines 1017-1066)
The _format_grouped_containers() method (lines 1156-1191) provides clean grouped output, automatically hiding empty sections.
flixopt/effects.py (3)
452-479: LGTM! Clean migration to ElementContainer framework.The EffectCollection properly integrates with the new container infrastructure:
- Inherits
ElementContainer[Effect]for type safety (line 452)- Initializes with descriptive element_type_name (line 458)
- Uses inherited
add()method (line 478) instead of direct dict manipulationThis provides consistent error handling and access patterns across the codebase.
524-530: LGTM! Validation now covers both sources and targets.The updated validation properly checks both sides of effect share mappings:
- Extracts all source and target effect names from temporal and periodic mappings
- Validates both sets against the container
- Raises descriptive error listing all unknown effects
This addresses the validation gap identified in previous reviews.
543-682: LGTM! Container interface consistently applied throughout.All collection access patterns properly delegate to the container framework:
__getitem__leverages parent's error handling with suggestions (line 559)__iter__returns keys viaself.keys()for dict-like behavior (line 565)__contains__supports both string labels and Effect objects (lines 567-573)- Loops updated to use
self.items()andself.values()(lines 609, 618, 668, 682)The implementation maintains consistency with the broader container-based architecture.
flixopt/results.py (6)
20-20: LGTM - Container mixin import.The import of
CompositeContainerMixinsupports the new unified dict-like access pattern across result containers.
57-57: LGTM - Container mixin inheritance.The inheritance enables unified dict-like access (
results['element_name']) across all result types, consistent with the PR's container architecture.
242-270: LGTM - Container initialization with flow data guards.The initialization correctly wraps result dictionaries in
ResultsContainerinstances and implements the_has_flow_dataflag to gracefully handle pre-v2.2.0 result files. This addresses the previous review concern about preventing crashes when flow data is absent.
567-568: LGTM - Flow data guard prevents crashes on old result files.The guard raises a clear error message when attempting to access flow data from pre-v2.2.0 result files, as recommended in the previous review.
630-631: LGTM - Flow data guard in sizes() method.Consistent with the guard in
flow_rates(), this prevents crashes when flow data is unavailable.
644-650: LGTM - Safe flow coordinate assignment.Converting
self.flows.values()to a list enables multiple iterations when building coordinates. Since this method is only called fromflow_rates()andsizes(), which both guard against missing flow data, this is safe from empty container access.
flixopt/results.py
Outdated
| def _get_container_groups(self) -> dict[str, dict]: | ||
| """Return ordered container groups for CompositeContainerMixin.""" | ||
| return { | ||
| 'Components': self.components, | ||
| 'Buses': self.buses, | ||
| 'Effects': self.effects, | ||
| 'Flows': self.flows, | ||
| } |
There was a problem hiding this comment.
Fix return type annotation to match base class.
The return type annotation dict[str, dict] doesn't match the actual return type (dict[str, ResultsContainer]) or the base class signature in CompositeContainerMixin (which uses dict[str, Any]).
Apply this diff:
- def _get_container_groups(self) -> dict[str, dict]:
+ def _get_container_groups(self) -> dict[str, Any]:
"""Return ordered container groups for CompositeContainerMixin."""🤖 Prompt for AI Agents
In flixopt/results.py around lines 287 to 294, the return type annotation
dict[str, dict] is incorrect; change it to match the base class signature by
annotating the method as returning dict[str, Any] (import Any from typing if not
already imported) so the signature aligns with CompositeContainerMixin and
reflects the heterogeneous container values.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
flixopt/results.py (1)
1127-1133: Critical: repr mutates object state.Line 1132 sets
sol.attrs = {}which mutatesself.solution.attrsbecausesol = self.solutioncreates a reference, not a copy.__repr__methods should never modify object state.Apply this diff to create a copy before clearing attrs:
def __repr__(self) -> str: """Return string representation with element info and dataset preview.""" class_name = self.__class__.__name__ header = f'{class_name}: "{self.label}"' - sol = self.solution + sol = self.solution.copy() sol.attrs = {} return f'{header}\n{"-" * len(header)}\n{repr(sol)}'
🧹 Nitpick comments (1)
flixopt/results.py (1)
285-292: Return type annotation could be more precise.The return type
dict[str, dict]doesn't precisely match the actual return values (ResultsContainer instances) or the base class signature (dict[str, Any]). While functional, consider aligning with the base class signature for consistency.Apply this diff to match the base class:
- def _get_container_groups(self) -> dict[str, dict]: + def _get_container_groups(self) -> dict[str, Any]: """Return ordered container groups for CompositeContainerMixin."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
flixopt/elements.py(5 hunks)flixopt/flow_system.py(12 hunks)flixopt/results.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
flixopt/flow_system.py (4)
flixopt/structure.py (12)
CompositeContainerMixin(1016-1191)Element(823-868)ElementContainer(989-998)FlowSystemModel(85-254)values(1142-1147)values(1423-1424)label_full(845-846)label_full(1308-1309)add(924-932)add(1426-1428)_format_grouped_containers(1156-1191)_get_container_groups(1068-1086)flixopt/elements.py (4)
Component(32-161)Bus(165-266)Flow(283-547)label_full(537-538)flixopt/effects.py (1)
EffectCollection(452-626)flixopt/io.py (1)
format_title_with_underline(836-846)
flixopt/results.py (3)
flixopt/structure.py (10)
CompositeContainerMixin(1016-1191)ResultsContainer(1001-1010)items(1149-1154)items(1417-1418)get(1270-1275)get(1430-1432)_get_container_groups(1068-1086)_format_grouped_containers(1156-1191)values(1142-1147)values(1423-1424)flixopt/flow_system.py (2)
flows(728-734)_get_container_groups(718-725)flixopt/io.py (1)
format_title_with_underline(836-846)
flixopt/elements.py (3)
flixopt/structure.py (2)
label_full(845-846)label_full(1308-1309)flixopt/io.py (2)
build_repr_from_init(629-807)format_flow_details(810-833)flixopt/interface.py (2)
InvestParameters(672-1084)format_for_repr(1054-1073)
⏰ 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). (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (22)
flixopt/elements.py (6)
14-14: LGTM! Import aligns with new formatting helpers.The fx_io import is used for the new
__repr__methods and formatting utilities.
93-94: LGTM! Proper initialization sequence.Checking unique labels before connecting flows ensures validation happens in the correct order.
121-156: LGTM! Robust flow attachment with validation.The method properly:
- Validates flow ownership before attachment
- Sets component reference and direction flags
- Deduplicates prevent_simultaneous_flows by object identity
- Rejects foreign flows with clear error messages
157-161: LGTM! Clean representation with flow details.The
__repr__uses helper utilities to show constructor parameters and flow information, making debugging easier.
264-266: LGTM! Consistent representation pattern.Extends base representation with flow details, matching the Component approach.
545-547: LGTM! Simple delegation to InvestParameters formatting.Wraps the InvestParameters repr with context, consistent with the representation improvements.
flixopt/flow_system.py (10)
9-9: LGTM! Imports support container-based refactoring.New imports enable the container-based access pattern and enhanced representations.
Also applies to: 16-16, 37-37
48-48: LGTM! Inheritance enables unified container access.The CompositeContainerMixin integration provides dict-like access patterns across all element types.
109-110: LGTM! Container types enable better access patterns.ElementContainer wrappers provide consistent interfaces and improve error messages.
Also applies to: 118-118
238-238: LGTM! Explicit iteration over effect values.Consistent with the container-based access pattern.
439-439: LGTM! Efficient iteration with chain.Using
chainand.values()provides clean iteration over multiple containers.Also applies to: 587-587
610-610: LGTM! Proper container operations with cache management.Container add operations correctly invalidate the flows cache, ensuring consistency.
Also applies to: 620-621, 627-628, 638-638
664-698: LGTM! Comprehensive and user-friendly representation.The
__repr__provides clear system overview with grouped container view, making inspection easier.
718-725: LGTM! Proper CompositeContainerMixin implementation.Returns ordered container groups for unified dict-like access.
728-734: LGTM! Efficient lazy flow collection with caching.Proper deduplication by object identity and deterministic sorting ensure consistency.
737-757: LGTM! Well-documented deprecation with migration path.Clear warning and examples guide users toward the new dict-like interface while maintaining backward compatibility.
flixopt/results.py (6)
20-20: LGTM! Container integration for results.Inheritance enables unified dict-like access across all result types.
Also applies to: 57-57
243-268: LGTM! Proper container initialization with backward compatibility.The
_has_flow_dataflag prevents errors when loading older result files missing flow information.
294-300: LGTM! Clear results representation.Provides identifying information and grouped container view for easy inspection.
565-566: LGTM! Proper guard against missing flow data.Clear error message helps users understand the limitation when loading older result files.
628-629: LGTM! Consistent guard pattern.Matches the flow_rates guard for consistent error handling across methods.
642-647: LGTM! Clean flow coordinate extraction.List conversion provides a stable view for coordinate assignment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
flixopt/results.py (1)
285-292: Type annotation mismatch with actual return type.Line 285 declares return type as
dict[str, ElementContainer], but the method actually returns a dict containingResultsContainerinstances (lines 288-291). While both inherit fromContainerMixin, this creates a type system inconsistency.This relates to the base class
CompositeContainerMixin._get_container_groupssignature in structure.py (line 1068), which also specifiesdict[str, ElementContainer]but should be more flexible.Consider using
dict[str, ResultsContainer]here to match the actual return, and loosening the base class signature todict[str, ContainerMixin]ordict[str, Any].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
flixopt/io.py(3 hunks)flixopt/results.py(8 hunks)flixopt/structure.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
flixopt/io.py (2)
flixopt/structure.py (2)
label_full(845-846)label_full(1308-1309)flixopt/config.py (1)
Modeling(184-195)
flixopt/structure.py (4)
flixopt/results.py (2)
flow_system(339-355)_get_container_groups(285-292)flixopt/elements.py (1)
label_full(537-538)flixopt/io.py (3)
format_sections_with_headers(864-878)build_repr_from_init(680-822)format_title_with_underline(851-861)flixopt/flow_system.py (1)
_get_container_groups(718-725)
flixopt/results.py (3)
flixopt/structure.py (12)
CompositeContainerMixin(1016-1191)ElementContainer(989-998)ResultsContainer(1001-1010)solution(159-182)items(1149-1154)items(1417-1418)get(1270-1275)get(1430-1432)_get_container_groups(1068-1086)_format_grouped_containers(1156-1191)values(1142-1147)values(1423-1424)flixopt/flow_system.py (2)
flows(728-734)_get_container_groups(718-725)flixopt/io.py (1)
format_title_with_underline(851-861)
⏰ 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). (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (19)
flixopt/io.py (7)
554-627: LGTM - Robust numeric formatting with proper edge case handling.The function correctly handles NaN, Inf, empty arrays, and nearly-uniform values. The use of
copy=Falseat line 598 is efficient and safe here since the array is only used for reading.
629-677: LGTM - Consistent value formatting with proper error handling.The function provides uniform formatting across different value types with appropriate truncation for readability.
680-822: LGTM - Comprehensive repr builder with proper edge case handling.The function introspects constructor signatures and intelligently filters parameters, with correct handling of label as positional, array comparisons, and special cases like
CONFIG.Modeling.big. The fallback at line 820 provides resilience.
825-848: LGTM - Clean flow details formatting.The function provides clear, conditionally formatted flow details with appropriate empty-string fallback.
851-861: LGTM - Simple and effective title formatting.Clean implementation that creates properly underlined titles.
864-878: LGTM - Consistent section formatting.The function correctly formats multiple sections with uniform header styling.
881-895: LGTM - Clean metadata string builder.The function appropriately filters empty parts and applies optional prefixing.
flixopt/structure.py (7)
871-877: LGTM - Correct natural sorting implementation.The regex-based natural sort key properly handles numeric components in strings for intuitive ordering (e.g., "bus1", "bus2", "bus10" instead of "bus1", "bus10", "bus2").
884-987: LGTM - Well-designed container with excellent UX.The ContainerMixin provides robust dict-like functionality with:
- Duplicate detection (lines 927-931)
- Label consistency validation (lines 937-941)
- Helpful error messages with suggestions via
get_close_matches(lines 960-971)- Natural sorting in repr (line 983)
989-999: LGTM - Clean ElementContainer implementation.Properly implements ContainerMixin using
label_fullfor element keying.
1001-1011: LGTM - ResultsContainer correctly uses plain label.Properly distinguishes from ElementContainer by using
element.labelinstead oflabel_fullfor Results objects.
1088-1191: LGTM - Comprehensive unified container access with excellent UX.The mixin provides:
- Unified search across containers with helpful error messages (lines 1088-1123)
- Complete dict-like interface (lines 1125-1154)
- Clean grouped representation (lines 1156-1191)
The implementation correctly aggregates operations across multiple typed containers while maintaining type safety for users.
848-850: LGTM - Consistent Element repr using centralized formatting.Properly leverages
fx_io.build_repr_from_initwith appropriate exclusions and theskip_default_sizeflag.
249-249: LGTM - Consistent use of fx_io formatting utilities.The repr methods across FlowSystemModel, Submodel, and Submodels correctly use the centralized formatting helpers for uniform output.
Also applies to: 355-355, 1355-1355, 1399-1399, 1408-1408
flixopt/results.py (5)
243-268: LGTM - Proper flow data handling with backward compatibility.The initialization correctly:
- Wraps all results in ResultsContainer for consistent access patterns (lines 247, 250, 253, 268)
- Guards against missing flow data in older result files (lines 255-262)
- Sets
_has_flow_dataflag for downstream checks (lines 262, 267)This addresses the past review concern about pre-v2.2.0 compatibility.
294-300: LGTM - Clean grouped representation for results.The repr effectively combines metadata (name, folder) with the grouped container view from
_format_grouped_containers, providing a clear overview of all result categories.
565-566: LGTM - Proper guards prevent crashes with old result files.The
_has_flow_datachecks with clear error messages (lines 565-566, 628-629) preventxr.concat([])crashes when flow data is absent in pre-v2.2.0 result files. This addresses the past review concern.Also applies to: 628-629
642-648: LGTM - Defensive list conversion ensures consistent iteration.Line 642 converts
flows.values()to a list before using it in coordinate assignment (lines 645-647). This ensures consistent behavior now thatflowsis aResultsContainerdict subclass.
1127-1133: LGTM - repr no longer mutates original dataset.Line 1131 uses
.copy(deep=False)before clearing attrs (line 1132), preventing unwanted mutation ofself.solution. This addresses the critical past review concern. Shallow copy is appropriate since only the attrs dict needs isolation.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(8 hunks)docs/user-guide/index.md(3 hunks)flixopt/effects.py(9 hunks)flixopt/flow_system.py(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/effects.py (1)
flixopt/structure.py (15)
Element(823-868)ElementContainer(989-998)ElementModel(1435-1456)FlowSystemModel(85-254)Submodel(1194-1368)add(924-932)add(1426-1428)keys(1138-1140)keys(1420-1421)label_full(845-846)label_full(1308-1309)items(1149-1154)items(1417-1418)values(1142-1147)values(1423-1424)
flixopt/flow_system.py (3)
flixopt/structure.py (11)
CompositeContainerMixin(1016-1191)Element(823-868)ElementContainer(989-998)values(1142-1147)values(1423-1424)label_full(845-846)label_full(1308-1309)add(924-932)add(1426-1428)_format_grouped_containers(1156-1191)_get_container_groups(1068-1086)flixopt/elements.py (4)
Component(32-161)Bus(165-266)Flow(283-547)label_full(537-538)flixopt/io.py (1)
format_title_with_underline(851-861)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
103-103: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
104-104: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
105-105: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
112-112: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
113-113: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
114-114: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
115-115: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
116-116: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
117-117: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
284-284: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
285-285: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
288-288: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
289-289: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
303-303: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
304-304: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
305-305: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
307-307: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
308-308: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
309-309: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
310-310: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
531-531: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
573-573: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
574-574: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
599-599: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
600-600: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
601-601: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
602-602: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
648-648: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
649-649: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
650-650: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
651-651: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
docs/user-guide/index.md
23-23: Reference links and images should use a label that is defined
Missing link or image reference definition: "flixopt.flowsystem.flowsystem"
(MD052, reference-links-images)
60-60: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
61-61: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
62-62: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
64-64: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
65-65: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
66-66: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
68-68: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
69-69: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
70-70: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
72-72: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
73-73: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
74-74: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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). (4)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
| if self._flows_cache is None: | ||
| flows = [f for c in self.components.values() for f in c.inputs + c.outputs] | ||
| # Deduplicate by id and sort for reproducibility | ||
| flows = sorted({id(f): f for f in flows}.values(), key=lambda f: f.label_full.lower()) | ||
| self._flows_cache = ElementContainer(flows, element_type_name='flows') | ||
| return self._flows_cache |
There was a problem hiding this comment.
Fix flows cache initialization to pass individual Flow objects.
ElementContainer expects each element to be passed individually so it can call .label_full. Supplying the list as a single argument causes an AttributeError the moment flow_system.flows is accessed, breaking every consumer of the new API. Expand the list when instantiating the container.
- self._flows_cache = ElementContainer(flows, element_type_name='flows')
+ self._flows_cache = ElementContainer(*flows, element_type_name='flows')🤖 Prompt for AI Agents
In flixopt/flow_system.py around lines 795 to 800, the code currently passes the
entire flows list as a single argument to ElementContainer which causes
AttributeError because ElementContainer expects each Flow as a separate
positional argument; fix it by expanding the flows list into individual
arguments when calling ElementContainer (i.e. pass each Flow object instead of
the list) and preserve the element_type_name='flows' argument so the container
can call .label_full on each element.
FBumann
changed the title
Description
Add Containers for Elements and results for better inspection and access patterns
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation