Feature/solution storage change+plotting acessors+deprecate plotting+coloring #509
Conversation
…y or Dataset instead of pandas DataFrame.
results.plot.variable(pattern)
Plots the same variable type across multiple elements for easy comparison.
# All binary operation states across all components
results.plot.variable('on')
# All flow_rates, filtered to Boiler-related elements
results.plot.variable('flow_rate', include='Boiler')
# All storage charge states
results.plot.variable('charge_state')
# With aggregation
results.plot.variable('flow_rate', aggregate='sum')
Key features:
- Searches all elements for variables matching the pattern
- Filter with include/exclude on element names
- Supports aggregation, faceting, and animation
- Returns Dataset with element names as variables
results.plot.duration_curve(variables)
Plots load duration curves (sorted time series) showing utilization patterns.
# Single variable
results.plot.duration_curve('Boiler(Q_th)|flow_rate')
# Multiple variables
results.plot.duration_curve(['CHP|on', 'Boiler|on'])
# Normalized x-axis (0-100%)
results.plot.duration_curve('demand', normalize=True)
Key features:
- Sorts values from highest to lowest
- Shows how often each power level is reached
- normalize=True shows percentage of time on x-axis
- Returns Dataset with duration_hours or duration_pct dimension
…r dimensions automatically
Summary of Fixes
I addressed the actionable code review comments from CodeRabbitAI:
1. Documentation Issue - reshape parameter name ✓ (No fix needed)
The CodeRabbitAI comment was incorrect. The public API parameter in PlotAccessor.heatmap() is correctly named reshape (line 335). The reshape_time parameter exists in the lower-level heatmap_with_plotly function, but the documentation correctly shows the public API parameter.
2. Fixed simple_example.py (lines 129-130)
Problem: The example called balance() and duration_curve() without required arguments, which would cause TypeError at runtime.
Fix: Added the required arguments:
- optimization.results.plot.balance('Fernwärme') - specifying the bus to plot
- optimization.results.plot.duration_curve('Boiler(Q_th)|flow_rate') - specifying the variable to plot
3. Fixed variable collision in plot_accessors.py (lines 985-988)
Problem: When building the Dataset in the variable() method, using element names as keys could cause collisions if multiple variables from the same element matched the pattern (e.g., 'Boiler|flow_rate' and 'Boiler|flow_rate_max' would both map to 'Boiler', with the latter silently overwriting the former).
Fix: Changed to use the full variable names as keys instead of just element names:
ds = xr.Dataset({var_name: self._results.solution[var_name] for var_name in filtered_vars})
All tests pass (40 passed, 1 skipped) and the example runs successfully.
| Method | facet_col | facet_row | |------------------|-------------------------------------------|-----------------------------| | balance() | 'scenario' | 'period' | | heatmap() | 'scenario' | 'period' | | storage() | 'scenario' | 'period' | | flows() | 'scenario' | 'period' | | effects() | 'scenario' | 'period' | | variable() | 'scenario' | 'period' | | duration_curve() | 'scenario' | 'period' (already had this) | | compare() | N/A (uses its own mode='overlay'/'facet') | | | sankey() | N/A (aggregates to single diagram) | | Removed animate_by parameter from all methods
| Method | Default mode | |------------------|---------------------------------------------------| | balance() | stacked_bar | | storage() | stacked_bar (flows) + line (charge state overlay) | | flows() | line | | variable() | line | | duration_curve() | line | | effects() | bar |
Changes made: 1. Period aggregation with weights: Uses period_weights from flow_system to properly weight periods by their duration 2. Scenario aggregation with weights: Uses normalized scenario_weights to compute a weighted average across scenarios 3. Selection support: Users can filter specific scenarios/periods via select parameter before aggregation Weighting logic: - Periods (for aggregate='sum'): (da * period_weights).sum(dim='period') - this gives the total energy across all periods weighted by their duration - Periods (for aggregate='mean'): (da * period_weights).sum(dim='period') / period_weights.sum() - weighted mean - Scenarios: Always uses normalized weights (sum to 1) for weighted averaging, since scenarios represent probability-weighted alternatives
|
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 WalkthroughThe changes introduce a carrier system for energy/material types with new Carrier and CarrierContainer classes, add centralized color management via ColorAccessor, extend Bus elements with carrier parameters, and update plotting functions to use context-aware coloring. Documentation and CSS styling updates support carrier-based color visualization. Changes
Sequence DiagramsequenceDiagram
actor User
participant FlowSystem as FlowSystem
participant Bus as Bus Element
participant CarrierMgmt as Carrier Management
participant ColorAccessor as ColorAccessor
participant Plotting as Plotting
User->>FlowSystem: create/setup with carriers
FlowSystem->>CarrierMgmt: initialize CarrierContainer
User->>Bus: create with carrier="heat"
Bus->>Bus: store carrier (lowercased)
User->>FlowSystem: connect_and_transform()
FlowSystem->>CarrierMgmt: _register_missing_carriers()
CarrierMgmt->>CarrierMgmt: auto-register from CONFIG.Carriers
rect rgb(200, 220, 240)
Note over User,Plotting: Color Resolution Flow
User->>ColorAccessor: setup(color_config)
ColorAccessor->>ColorAccessor: store mappings
User->>Plotting: plot() with colors=None
Plotting->>FlowSystem: request colors property
FlowSystem->>ColorAccessor: lazy initialize
Plotting->>ColorAccessor: for_bus("heat_bus")
ColorAccessor->>CarrierMgmt: resolve carrier
CarrierMgmt-->>ColorAccessor: return Carrier with color
ColorAccessor-->>Plotting: return hex color
Plotting->>Plotting: apply context-aware coloring
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 base branch from
feature/solution-storage-change
to
feature/solution-storage-change+plotting-acessors+deprecate-plotting
FBumann
changed the base branch from
feature/solution-storage-change+plotting-acessors+deprecate-plotting
to
feature/solution-storage-change+plotting-acessors
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
… feature/solution-storage-change+plotting-acessors+deprecate-plotting+coloring # Conflicts: # flixopt/clustering.py # flixopt/statistics_accessor.py # flixopt/topology_accessor.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
flixopt/topology_accessor.py (2)
24-99: Code duplication withplotting.plot_network.This function is nearly identical to
plot_networkinflixopt/plotting.py(lines 775-856). Consider consolidating by having one delegate to the other, or extracting to a shared utility to avoid maintaining duplicate code.-def _plot_network( - node_infos: dict, - edge_infos: dict, - path: str | pathlib.Path | None = None, - controls: bool - | list[ - Literal['nodes', 'edges', 'layout', 'interaction', 'manipulation', 'physics', 'selection', 'renderer'] - ] = True, - show: bool = False, -) -> pyvis.network.Network | None: - """Visualize network structure using PyVis. - ... - """ - # ... duplicate implementation ... +from .plotting import plot_network as _plot_network
78-85: Simplify conditional logic for save/show handling.The
elif pathandelif showbranches both write HTML, creating some redundancy. Consider restructuring:if not show and not path: return net - elif path: - path = pathlib.Path(path) if isinstance(path, str) else path - net.write_html(path.as_posix()) - elif show: + + if path: + path = pathlib.Path(path) if isinstance(path, str) else path + else: path = pathlib.Path('network.html') - net.write_html(path.as_posix()) + net.write_html(path.as_posix())flixopt/config.py (1)
578-606: Minor: Import placement inside class body.The
Carrierimport at line 598 is placed inside theCONFIG.Carriersclass body. While this works, it's unconventional.Consider moving the import to the module level for consistency:
from __future__ import annotations import logging import os import warnings from logging.handlers import RotatingFileHandler from pathlib import Path from types import MappingProxyType from typing import TYPE_CHECKING, Literal + +from .carrier import Carrier ... class Carriers: """Default carrier definitions for common energy types. ... """ - from .carrier import Carrier - # Default carriers - colors from D3/Plotly palettes electricity: Carrier = Carrier('electricity', '#FECB52') # YellowHowever, the current implementation works correctly and may avoid circular import issues.
flixopt/flow_system.py (2)
689-704: Hardcoded default values in carrier restoration could diverge from Carrier defaults.When restoring carriers from serialized data, the defaults
'#808080'for color,'kW'for unit, and''for description are hardcoded. If theCarrierclass defaults change, these won't be synchronized.Consider using the
Carrierclass defaults or the_resolve_reference_structurepattern used for other elements:- carrier = Carrier( - name=carrier_data.get('name', ''), - color=carrier_data.get('color', '#808080'), - unit=carrier_data.get('unit', 'kW'), - description=carrier_data.get('description', ''), - ) + carrier = cls._resolve_reference_structure(carrier_data, arrays_dict)Or at minimum, extract defaults to constants shared with the
Carrierclass.
945-970: Docstring mentions combined access but method only returns local carriers.The
get_carrierdocstring mentions "After connect_and_transform(), this includes carriers auto-registered from CONFIG.Carriers", which is accurate. However, thecarriersproperty docstring says "For combined access (local + CONFIG.Carriers), use get_carrier()" butget_carrieralso only accessesself._carriers. The auto-registration happens duringconnect_and_transform, so the behavior is correct, but the wording could be clearer.Consider clarifying the docstring:
def get_carrier(self, name: str) -> Carrier | None: """Get a carrier by name. - Returns carriers registered on this FlowSystem. After connect_and_transform(), - this includes carriers auto-registered from CONFIG.Carriers for buses that - reference them. + Returns carriers from this FlowSystem's local registry. Carriers are added via: + - Explicit registration with add_carrier() + - Auto-registration from CONFIG.Carriers during connect_and_transform() for + buses that reference unregistered carriersflixopt/color_accessor.py (2)
66-70: Consider using weak reference to avoid potential memory leak.The
ColorAccessorholds a reference to theFlowSystem, andFlowSystemholds a reference toColorAccessorvia_colors. While Python's garbage collector handles cycles, using a weak reference could be cleaner.This is a minor consideration and not strictly necessary given Python's cyclic GC:
import weakref def __init__(self, flow_system: FlowSystem) -> None: self._fs_ref = weakref.ref(flow_system) # ... @property def _fs(self) -> FlowSystem: fs = self._fs_ref() if fs is None: raise RuntimeError("FlowSystem has been garbage collected") return fs
101-119: Label type detection heuristic may misclassify edge cases.The logic
label.islower()treats any all-lowercase label as a carrier, but component/bus labels could also be lowercase. The fallback to_component_colorsfor unrecognized labels is reasonable, but could lead to unexpected behavior if a user has a lowercase component name like"boiler".Consider checking carriers first against
CONFIG.Carriers, then checking against actual registered elements:for label, color in config.items(): - # Check if it's a known carrier (has attribute on CONFIG.Carriers or lowercase) - if hasattr(CONFIG.Carriers, label) or label.islower(): + # Check if it's a known carrier in CONFIG.Carriers + if hasattr(CONFIG.Carriers, label) or hasattr(CONFIG.Carriers, label.lower()): self._carrier_colors[label] = color # Check if it's a component elif label in self._fs.components: self._component_colors[label] = color # Check if it's a bus elif label in self._fs.buses: self._bus_colors[label] = color - # Otherwise treat as component (most common case) + # Otherwise treat as component (most common case for forward declarations) else: self._component_colors[label] = color
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
docs/stylesheets/extra.css(1 hunks)docs/user-guide/core-concepts.md(1 hunks)docs/user-guide/mathematical-notation/elements/Bus.md(1 hunks)docs/user-guide/results-plotting.md(1 hunks)flixopt/__init__.py(2 hunks)flixopt/carrier.py(1 hunks)flixopt/clustering.py(4 hunks)flixopt/color_accessor.py(1 hunks)flixopt/color_processing.py(1 hunks)flixopt/config.py(2 hunks)flixopt/elements.py(3 hunks)flixopt/flow_system.py(10 hunks)flixopt/plotting.py(1 hunks)flixopt/statistics_accessor.py(24 hunks)flixopt/topology_accessor.py(2 hunks)tests/test_solution_and_plotting.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
flixopt/__init__.py (1)
flixopt/carrier.py (2)
Carrier(13-124)CarrierContainer(127-158)
flixopt/plotting.py (1)
flixopt/color_processing.py (1)
process_colors(112-180)
tests/test_solution_and_plotting.py (1)
flixopt/plotting.py (2)
reshape_data_for_heatmap(604-772)heatmap_with_plotly_v2(1145-1193)
flixopt/flow_system.py (3)
flixopt/color_accessor.py (3)
ColorAccessor(19-403)to_dict(370-380)from_dict(383-397)flixopt/carrier.py (2)
Carrier(13-124)CarrierContainer(127-158)flixopt/config.py (3)
to_dict(650-678)CONFIG(181-854)Carriers(578-606)
flixopt/config.py (2)
flixopt/carrier.py (1)
Carrier(13-124)tests/conftest.py (3)
electricity(84-85)heat(88-89)gas(92-93)
flixopt/color_accessor.py (4)
flixopt/color_processing.py (1)
process_colors(112-180)flixopt/config.py (4)
CONFIG(181-854)Carriers(578-606)Plotting(547-576)to_dict(650-678)flixopt/flow_system.py (3)
FlowSystem(49-1954)get_carrier(945-958)flows(1470-1476)flixopt/io.py (1)
load_yaml(137-154)
flixopt/clustering.py (2)
flixopt/flow_system.py (1)
colors(1178-1212)flixopt/results.py (2)
colors(2147-2148)colors(2151-2155)
⏰ 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.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (41)
flixopt/clustering.py (2)
31-32: LGTM!Moving
pathlibunderTYPE_CHECKINGis the correct approach since it's only used for type hints in the function signature.
148-184: LGTM! Clean refactor to use plotly.express directly.The plotting logic is well-structured. A minor note: when
saveis provided andshowis True, both operations correctly execute independently.flixopt/topology_accessor.py (1)
219-221: LGTM!Clean delegation to the extracted
_plot_networkfunction.flixopt/color_processing.py (1)
18-67: LGTM! Well-documented type alias.The
ColorTypealias with comprehensive documentation provides excellent guidance for users on supported color input formats. The examples cover all use cases clearly.flixopt/plotting.py (1)
42-42: LGTM! Good centralization ofColorType.Importing
ColorTypefromcolor_processingeliminates duplication and ensures consistent type definitions across the codebase.docs/user-guide/core-concepts.md (1)
34-44: LGTM! Clear documentation for the new carrier concept.The documentation effectively introduces carriers as a semantic and visualization enhancement for Buses, with helpful code examples. The cross-reference link to Color Management is valid and points to the appropriate section.
docs/user-guide/results-plotting.md (1)
321-427: LGTM! Comprehensive color management documentation.The Color Management section is well-structured and covers all key aspects:
- Clear carrier definitions with visual color indicators
- Practical examples for common use cases
- Well-defined color resolution priority
- Good integration with FlowSystem API
The inline color swatches (line 331-336) depend on CSS styling defined in
docs/stylesheets/extra.css.tests/test_solution_and_plotting.py (1)
350-356: LGTM! Test updated to match new heatmap API.The test correctly reflects the new usage pattern where
reshape_data_for_heatmapis called separately before passing data toheatmap_with_plotly_v2. This separates data preparation from visualization, improving modularity.docs/stylesheets/extra.css (2)
766-784: LGTM! Color swatch styling for documentation.The
.color-swatchclass provides clean inline color indicators for the documentation. The implementation includes:
- Appropriate sizing (1em × 1em)
- Border and shadow for visibility
- Dark mode support
This aligns with the color indicators used in the Color Management documentation section.
786-812: Footer alignment updates.Footer styling changes hide the metadata section and adjust grid padding across breakpoints. These changes appear unrelated to the carrier/color management feature but improve documentation layout consistency.
flixopt/__init__.py (1)
17-17: LGTM! Public API exposure for Carrier types.The
CarrierandCarrierContainerclasses are properly exposed through the package's public API by:
- Importing from
flixopt.carriermodule- Adding to
__all__for explicit exportThis allows users to access these types as
fx.Carrierandfx.CarrierContainer.Also applies to: 38-39
flixopt/elements.py (2)
197-232: LGTM! Well-documented carrier parameter addition.The Bus documentation has been updated to include the new
carrierparameter with:
- Clear explanation of its purpose (automatic coloring, unit tracking, semantic grouping)
- Practical examples for both predefined and custom carriers
- References to carrier registration via FlowSystem
The documentation effectively guides users on how to use this new feature.
251-261: LGTM! Carrier parameter implementation.The carrier parameter is properly implemented:
- Added to
Bus.__init__signature with optional default (line 251)- Normalized to lowercase for consistent string matching (line 261)
- Stored as
self.carrierfor later useThe lowercase normalization aligns with the
Carrierclass implementation which also normalizes names to lowercase.flixopt/config.py (2)
601-606: LGTM! Predefined carrier defaults with appropriate colors.The six predefined carriers use colors from D3/Plotly palettes, providing professional and consistent defaults:
electricity: Yellow (#FECB52) - evokes energy/lightningheat: Red (#D62728) - evokes warmth/firegas: Blue (#1F77B4) - evokes natural gashydrogen: Purple (#9467BD) - modern/clean energyfuel: Brown (#8C564B) - fossil fuels/oilbiomass: Green (#2CA02C) - organic/renewable
634-642: LGTM! Reset method updated for Carriers.The
reset()method correctly reinitializes all six carrier defaults to their original values, ensuring consistent behavior after configuration changes.flixopt/carrier.py (4)
13-93: LGTM! Well-designed Carrier class.The
Carrierclass is well-implemented with:
- Comprehensive docstring with practical examples (lines 22-73)
- Name normalization to lowercase for case-insensitive matching (line 90)
- Clear attribute storage (color, unit, description)
- Inheritance from
Interfacefor serialization supportThe design allows carriers to be referenced by name string or object interchangeably.
95-103: LGTM! Appropriate no-op transform_data.The
transform_datamethod is correctly implemented as a no-op since carriers represent metadata without time-series data. This aligns with theInterfacecontract while avoiding unnecessary computation.
110-118: LGTM! Name-based equality enables flexible usage.The equality implementation allows carriers to be compared by:
- Carrier object to Carrier object (line 114-115)
- Carrier object to string (line 116-117)
Both comparisons use lowercase names for case-insensitive matching, enabling flexible usage patterns where carrier strings and objects are interchangeable.
127-158: LGTM! Container provides dict-like access.The
CarrierContainerclass properly:
- Inherits from
ContainerMixin['Carrier']for consistent container behavior- Accepts flexible initialization (list or dict) (line 148)
- Uses carrier name for keying via
_get_label(lines 156-158)- Provides clear docstring with usage examples (lines 133-145)
docs/user-guide/mathematical-notation/elements/Bus.md (1)
5-26: LGTM! Clear and helpful documentation addition.The new Carriers subsection provides clear explanations and practical examples. The documentation effectively introduces the carrier concept with its benefits (automatic coloring, unit tracking, semantic grouping). The link to the Color Management section is correctly formatted and the target exists.
flixopt/flow_system.py (7)
7-7: LGTM! Import additions are appropriate.The new imports for
json,ColorAccessor,Carrier, andCarrierContainerare correctly placed and support the new color management and carrier functionality.Also applies to: 20-20, 44-44
224-228: LGTM! Instance attributes properly initialized.The initialization of
_colorsasNone(lazy initialization) and_carriersas an emptyCarrierContainerfollows good patterns for optional cached state.
858-858: LGTM! Carrier registration called at appropriate point.Calling
_register_missing_carriers()after_connect_network()ensures buses are connected before carrier resolution, which is the correct order of operations.
915-943: LGTM! Well-documented public API for carrier registration.The
add_carriermethod has clear docstring, proper type checking, and a good usage example. The error handling for non-Carrier types is appropriate.
1177-1212: LGTM! Excellent documentation for the colors property.The docstring thoroughly explains the ColorAccessor's context-aware behavior and provides clear examples for both configuration and usage scenarios.
867-875: The carrier comparison logic works correctly as implemented.The check
bus.carrier not in self._carrierscorrectly compares a string againstCarrierContainerkeys, which are keyed bycarrier.name(string). Thegetattr(CONFIG.Carriers, bus.carrier, None)pattern works as designed:CONFIG.Carriershas attributes named after the carrier identifiers (e.g.,.electricity,.heat,.gas), matching the string values stored inbus.carrier. When a default carrier is retrieved, it's registered withself._carriers[bus.carrier] = default_carrier, which uses the string key matching the container's keying convention.
591-604: Carrier serialization pattern is appropriate and requires no changes.Carriers are simple metadata-only objects with only scalar string attributes (name, color, unit, description) and explicitly have no time-series data or DataArray content. The second return value from
_create_reference_structure()is always empty for Carrier objects, making the_discard pattern semantically correct and intentional. Unlike components, buses, and effects which may contain complex nested structures with extracted arrays, carriers require no array extraction or aggregation. The pattern difference is justified by the fundamental difference in data structure complexity.flixopt/color_accessor.py (6)
1-17: LGTM! Clean module structure with proper TYPE_CHECKING guard.The imports are well-organized with
FlowSystemproperly guarded for type checking only, avoiding circular imports.
161-185: LGTM! Clean resolution chain with proper fallback.The
for_componentmethod correctly implements the priority order: explicit colors → meta_data → None. The guard checkif label in self._fs.componentsprevents KeyError.
246-272: Potential issue:flow.bustype handling assumes string or object withlabel.Line 271 handles
flow.busas either a string or an object with alabelattribute. This is defensive coding, but ifflow.busis neither, it could fail silently or produce unexpected results.
274-319: LGTM! Robust color map builder with fallback colorscale.The
get_color_map_for_balancecorrectly separates flows with configured colors from those needing fallback, ensuring complete coverage. The context detection based on node type is clean.
321-362: LGTM! Sankey color map handles both buses and components.The resolution order (bus → component → fallback) is appropriate for Sankey diagrams where nodes are mixed element types.
382-397: LGTM! Clean serialization round-trip implementation.The
from_dictmethod properly uses.copy()to avoid sharing mutable state between instances.flixopt/statistics_accessor.py (8)
29-29: LGTM! Proper imports for new color handling.The imports of
plotly.express as px(moved to module level) andColorType, process_colorsfromcolor_processingare appropriate for the centralized color management.Also applies to: 33-33
51-118: Robust time reshaping with good format validation.The
_reshape_time_for_heatmapfunction has a well-defined set of supported format pairs and raises a clear error for unsupported combinations. The special handling for Sunday ('0_Sunday'→'7_Sunday') ensures proper weekday ordering.One minor consideration: the
resampled.ffill()call could fill NaNs across large gaps if data is sparse.
121-165: LGTM! Clean heatmap figure builder.The
_heatmap_figurefunction properly handles empty data, applies defaults from CONFIG, and conditionally adds faceting/animation arguments.
269-297: LGTM! Updated to use ColorType and process_colors.The
_create_stacked_barfunction correctly usesprocess_colorsto convert the flexibleColorTypeinput into a concrete color map for plotly.
740-824: LGTM! Context-aware color integration in balance plot.The
balancemethod correctly usesself._fs.colors.get_color_map_for_balance()when no colors are provided, enabling automatic context-aware coloring (component colors for bus balance, carrier colors for component balance).
826-935: LGTM! Heatmap uses new internal helpers correctly.The heatmap method properly delegates to
_reshape_time_for_heatmapand_heatmap_figure, with appropriate dimension handling and facet resolution.
1022-1138: LGTM! Sankey uses ColorAccessor for bus-based coloring.The sankey method correctly uses
self._fs.colors.get_color_map_for_sankey()when colors are not specified, ensuring buses get carrier-based colors automatically.
1279-1416: LGTM! Effects plotting correctly uses process_colors.The
effectsmethod properly processes colors through the unifiedprocess_colorsfunction, maintaining consistency with other plotting methods.
flixopt/color_accessor.py
Outdated
| def for_carrier(self, carrier: str) -> str | None: | ||
| """Get color for a carrier. | ||
|
|
||
| Resolution order: | ||
| 1. Explicit carrier color override from setup() | ||
| 2. FlowSystem-registered carrier (via add_carrier()) | ||
| 3. CONFIG.Carriers default | ||
| 4. None if carrier not found | ||
|
|
||
| Args: | ||
| carrier: Carrier name. | ||
|
|
||
| Returns: | ||
| Color string or None if not configured. | ||
| """ | ||
| carrier_lower = carrier.lower() | ||
|
|
||
| # Check explicit color override | ||
| if carrier_lower in self._carrier_colors: | ||
| return self._carrier_colors[carrier_lower] | ||
|
|
||
| # Check FlowSystem-registered carriers | ||
| carrier_obj = self._fs.get_carrier(carrier_lower) | ||
| if carrier_obj: | ||
| return carrier_obj.color | ||
|
|
||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "CONFIG\.Carriers" --type py -C 2Repository: flixOpt/flixopt
Length of output: 4867
🏁 Script executed:
rg -n "def get_carrier" --type py -A 10Repository: flixOpt/flixopt
Length of output: 724
🏁 Script executed:
rg -n "def get_carrier" --type py -A 15Repository: flixOpt/flixopt
Length of output: 983
🏁 Script executed:
fd -e py -type f | xargs rg -l "for_carrier" | head -5Repository: flixOpt/flixopt
Length of output: 272
🏁 Script executed:
fd "test" -type dRepository: flixOpt/flixopt
Length of output: 229
🏁 Script executed:
fd test --type dRepository: flixOpt/flixopt
Length of output: 65
🏁 Script executed:
fd "color" tests/ --type fRepository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
rg "for_carrier" --type py -B 3 -A 3Repository: flixOpt/flixopt
Length of output: 594
🏁 Script executed:
rg "def for_color" --type py -A 20 | head -40Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
rg "if bus.carrier:" --type py -B 5 -A 5Repository: flixOpt/flixopt
Length of output: 676
🏁 Script executed:
rg "def for_" flixopt/color_accessor.py -A 25 | head -60Repository: flixOpt/flixopt
Length of output: 1866
🏁 Script executed:
sed -n '218,244p' flixopt/color_accessor.pyRepository: flixOpt/flixopt
Length of output: 871
🏁 Script executed:
sed -n '100,115p' flixopt/color_accessor.pyRepository: flixOpt/flixopt
Length of output: 725
🏁 Script executed:
rg "getattr(CONFIG.Carriers" --type py -B 2 -A 2Repository: flixOpt/flixopt
Length of output: 140
🏁 Script executed:
rg "CONFIG\.Carriers.*getattr" --type py -B 2 -A 2Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
rg -B 5 "def for_carrier" flixopt/color_accessor.py | head -20Repository: flixOpt/flixopt
Length of output: 216
for_carrier() missing CONFIG.Carriers fallback promised in docstring.
The method only checks self._carrier_colors (explicit overrides) and self._fs.get_carrier() (FlowSystem-registered carriers), then returns None. However, the docstring lists "CONFIG.Carriers default" as resolution step 3. The method should fall back to getattr(CONFIG.Carriers, carrier, None) before returning None, matching the documented behavior.
🤖 Prompt for AI Agents
In flixopt/color_accessor.py around lines 218 to 244, the for_carrier()
implementation skips the "CONFIG.Carriers default" fallback promised in the
docstring; add a lookup to return getattr(CONFIG.Carriers, carrier, None) (or
getattr(CONFIG.Carriers, carrier_lower, None) if CONFIG stores lowercased
attributes) after checking FlowSystem and before returning None so the method
follows the documented resolution order and returns the configured default color
when present.
…storage-change+plotting-acessors+deprecate-plotting+coloring # Conflicts: # docs/user-guide/results-plotting.md # examples/01_Simple/simple_example.py # examples/02_Complex/complex_example.py # examples/04_Scenarios/scenario_example.py # flixopt/flow_system.py # flixopt/statistics_accessor.py
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.