Feature/rename parameters linear converters #468
Conversation
|
Warning Rate limit exceeded@FBumann has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughRenames legacy converter parameters in Changes
Sequence Diagram(s)sequenceDiagram
actor User as User code
participant Init as Converter.__init__
participant DepMap as _handle_deprecated_kwarg
participant Validate as _validate_kwargs
participant Assign as assign attrs / conversion_factors
participant DepProp as deprecated property access
Note over User,Init: instantiate with new or legacy kwargs
User->>Init: __init__(label, ...kwargs)
Init->>DepMap: detect legacy kwargs → map to new names (emit DeprecationWarning)
DepMap-->>Init: normalized kwargs
Init->>Validate: verify allowed kwargs
Validate-->>Init: validated kwargs
Init->>Assign: set thermal_efficiency / fuel_flow / electrical_flow / thermal_flow / cop ...
Note right of Assign: canonical attributes are descriptive lowercase
User->>DepProp: access old-name property (e.g., `Q_fu` / `COP`)
DepProp-->>User: emit DeprecationWarning, forward to new attribute
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/linear_converters.py (1)
80-105: Required flows are allowed to beNone, leading to late failuresAcross
Boiler,Power2Heat,HeatPump,CoolingTower,CHP, andHeatPumpWithSource, the key flow arguments are declared asFlow | None = Nonebut are used immediately via.labeland passed intoinputs/outputs. If a caller forgets to pass one of these flows (or passesNone), this will fail later with anAttributeErrorinstead of an early, clear error.Consider either:
- Making these parameters required again (no default
None), or- Adding explicit validation in
__init__(e.g.,if fuel_flow is None: raise ValueError("fuel_flow is required")) before using.label.This would fail fast and give a more actionable error for misconfigured components.
Also applies to: 193-219, 307-334, 432-458, 555-587, 706-735
🧹 Nitpick comments (1)
flixopt/linear_converters.py (1)
787-819: Improve logging incheck_boundsto avoid verbose array dumpsThe current implementation logs full arrays via
{parameter_label}={value}, which produces extremely verbose output whenvalueis a largeTimeSeriesDataobject (e.g., 8760+ timesteps or multiple scenarios).Consider these improvements:
- Log array shape and summary statistics (min/max) instead of the full array; only include a small sample or statistics as shown in scientific Python best practices.
- Optionally add a rate-limit or deduplication mechanism to prevent log floods if bounds violations occur frequently.
Example: replace
{parameter_label}={value}with{parameter_label}.shape={np.asarray(value).shape}and the statistics already logged vianp.min/max.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)flixopt/linear_converters.py(30 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/linear_converters.py (1)
flixopt/structure.py (2)
_handle_deprecated_kwarg(420-479)_validate_kwargs(481-509)
⏰ 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.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
🔇 Additional comments (1)
CHANGELOG.md (1)
73-83: Changelog entries align with implementationThe parameter renaming and deprecation bullets correctly mirror the changes in
linear_converters.py(new lowercase names plus deprecation ofQ_fu,Q_th,P_el,COP,Q_ab). Looks consistent and clear.
# Conflicts: # flixopt/linear_converters.py
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/linear_converters.py (1)
304-448: Constructor backward compatibility is broken for deprecatedCOPparameterThe issue is confirmed. Both
HeatPump.__init__(line 356) andHeatPumpWithSource.__init__(line 847) declarecop: Numeric_TPSas a required positional parameter without a default. Calls using the deprecatedCOP=kwarg (e.g.,HeatPump(label='hp', COP=3.5, power_flow=..., thermal_flow=...)) fail withTypeError: missing required positional argument: 'cop'before the function body executes and before_handle_deprecated_kwargcan remap the deprecated parameter.To restore backward compatibility,
copmust have a default value so the deprecated kwarg can be captured and remapped. The fix matches the suggestion in the review comment:
- Change
cop: Numeric_TPStocop: Numeric_TPS | None = None- Add validation:
if cop is None: raise ValueError(...)This applies to both classes at lines 353–385 and 844–896 respectively.
🧹 Nitpick comments (1)
flixopt/linear_converters.py (1)
971-1017: Usevalue_arrfor comparisons incheck_boundsfor more robust typing
check_boundsnow buildsvalue_arr = np.asarray(value)(Line 996) but still performs the comparisons onvalue(Lines 998, 1008). This works for scalars and NumPy arrays, but is brittle for other array‑likes (e.g. lists) wherevalue > lower_boundmay not be defined, despitenp.asarray(value)succeeding.Since you already normalize
valueintovalue_arr(and log stats from it), it’s safer and more consistent to use it for the comparisons as well:- # Convert to array for shape and statistics - value_arr = np.asarray(value) - - if not np.all(value > lower_bound): + # Convert to array for shape and statistics + value_arr = np.asarray(value) + + if not np.all(value_arr > lower_bound): @@ - if not np.all(value < upper_bound): + if not np.all(value_arr < upper_bound):This keeps semantics the same for NumPy/TimeSeriesData inputs while avoiding type issues for other array‑likes and tying the comparisons to exactly what you log.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
flixopt/linear_converters.py(30 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/linear_converters.py (2)
flixopt/structure.py (2)
_handle_deprecated_kwarg(415-474)_validate_kwargs(476-504)flixopt/core.py (1)
TimeSeriesData(33-141)
🔇 Additional comments (4)
flixopt/linear_converters.py (4)
7-7: Boiler refactor, deprecated kwargs, and Q_ accessors look consistent*
warningsimport, doc updates, and the switch tofuel_flow/thermal_floware coherent with the new API._handle_deprecated_kwarg+_validate_kwargscorrectly supportQ_fu/Q_thwhile rejecting unknown extras, and explicitNonechecks ensure required flows are provided.etagetter/setter wiring viaconversion_factorsis internally consistent, and the deprecatedQ_fu/Q_thproperties now have symmetric getters and setters forwarding to the new attributes withDeprecationWarning.Also applies to: 32-153
166-291: Power2Heat parameter rename and deprecation handling are well wired
- The constructor now cleanly prefers
power_flow/thermal_flowwhile still acceptingP_el/Q_thvia_handle_deprecated_kwarg, followed by_validate_kwargsand explicitNonechecks for required flows.etais exposed throughconversion_factorsin the same pattern asBoiler, and deprecatedP_el/Q_thproperties (with setters) correctly forward to the new attributes with warnings.
451-587: CoolingTower: renamed flows and specific_electricity_demand mapping are coherent
- Constructor uses
_handle_deprecated_kwargforP_el/Q_thto map ontopower_flow/thermal_flow, then validates required flows and rejects unexpected kwargs via_validate_kwargs.specific_electricity_demandgetter/setter useconversion_factors = [{power_flow: -1, thermal_flow: value}], which matches the documented relationshippower_flow = thermal_flow × specific_electricity_demand.- Deprecated
P_el/Q_thaccessors mirror the pattern used elsewhere and should maintain BC for attribute access and assignment.
590-771: CHP: flow renames, eta wiring, and deprecated proxies are consistent
- The constructor correctly maps
Q_fu/P_el/Q_thtofuel_flow/power_flow/thermal_flowvia_handle_deprecated_kwarg, then enforces that all three flows are non‑Noneand validates extra kwargs.eta_thandeta_elgetters/setters use twoconversion_factorsrows, and the combinedcheck_bounds(eta_el + eta_th, ...)keeps the overall efficiency in check while only logging if exceeded.- Deprecated
Q_fu/P_el/Q_thproperties (with setters) forward to the new attributes withDeprecationWarning, aligning with the rest of the module.
Robust Type Handling in check_bounds
Renamed eta_th → thermal_efficiency Renamed eta_el → electrical_efficiency - Fixed to use value_arr for comparisons instead of value
…ters-linear-converters # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_functional.py (1)
86-93: Consider using keyword arguments for clarity.The code works correctly, but mixing positional (
0.5forthermal_efficiency) and keyword arguments could reduce clarity. Consider usingthermal_efficiency=0.5explicitly.Apply this pattern throughout the file:
fx.linear_converters.Boiler( 'Boiler', - 0.5, + thermal_efficiency=0.5, fuel_flow=fx.Flow('Q_fu', bus='Gas'), thermal_flow=fx.Flow('Q_th', bus='Fernwärme'), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
examples/00_Minmal/minimal_example.py(1 hunks)examples/01_Simple/simple_example.py(1 hunks)examples/02_Complex/complex_example.py(2 hunks)examples/03_Calculation_types/example_calculation_types.py(2 hunks)examples/04_Scenarios/scenario_example.py(1 hunks)examples/05_Two-stage-optimization/two_stage_optimization.py(2 hunks)tests/conftest.py(5 hunks)tests/test_component.py(3 hunks)tests/test_effect.py(1 hunks)tests/test_flow_system_resample.py(1 hunks)tests/test_functional.py(27 hunks)tests/test_integration.py(2 hunks)tests/test_scenarios.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
tests/test_integration.py (4)
flixopt/elements.py (1)
flow_rate(677-679)flixopt/results.py (1)
flow_rate(1938-1939)flixopt/structure.py (3)
solution(154-177)values(1194-1199)values(1476-1477)tests/conftest.py (1)
assert_almost_equal_numeric(691-703)
examples/00_Minmal/minimal_example.py (2)
flixopt/linear_converters.py (6)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)flixopt/elements.py (1)
Flow(294-560)
examples/04_Scenarios/scenario_example.py (2)
flixopt/linear_converters.py (9)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)CHP(637-869)electrical_efficiency(773-774)electrical_efficiency(777-779)flixopt/elements.py (1)
Flow(294-560)
examples/01_Simple/simple_example.py (2)
flixopt/linear_converters.py (9)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)CHP(637-869)electrical_efficiency(773-774)electrical_efficiency(777-779)flixopt/elements.py (1)
Flow(294-560)
examples/05_Two-stage-optimization/two_stage_optimization.py (1)
flixopt/linear_converters.py (8)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)electrical_efficiency(773-774)electrical_efficiency(777-779)
tests/conftest.py (3)
flixopt/linear_converters.py (10)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)Boiler(24-175)electrical_efficiency(773-774)electrical_efficiency(777-779)CHP(637-869)flixopt/elements.py (1)
Flow(294-560)flixopt/interface.py (1)
OnOffParameters(1080-1341)
tests/test_effect.py (2)
flixopt/linear_converters.py (6)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)flixopt/elements.py (1)
Flow(294-560)
tests/test_functional.py (2)
flixopt/elements.py (3)
Flow(294-560)on_off(758-762)flow_rate(677-679)flixopt/linear_converters.py (1)
Boiler(24-175)
tests/test_flow_system_resample.py (1)
flixopt/elements.py (1)
Flow(294-560)
examples/02_Complex/complex_example.py (3)
flixopt/linear_converters.py (9)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)CHP(637-869)electrical_efficiency(773-774)electrical_efficiency(777-779)flixopt/interface.py (1)
OnOffParameters(1080-1341)flixopt/elements.py (1)
Flow(294-560)
tests/test_component.py (1)
flixopt/linear_converters.py (7)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)Boiler(24-175)
tests/test_scenarios.py (2)
flixopt/linear_converters.py (6)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)flixopt/elements.py (1)
Flow(294-560)
examples/03_Calculation_types/example_calculation_types.py (2)
flixopt/linear_converters.py (8)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)electrical_efficiency(773-774)electrical_efficiency(777-779)flixopt/interface.py (1)
OnOffParameters(1080-1341)
⏰ 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.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (20)
examples/05_Two-stage-optimization/two_stage_optimization.py (2)
46-60: LGTM! Boiler parameters correctly updated to new API.The Boiler instantiation has been properly updated to use the new parameter names (
thermal_efficiency,thermal_flow,fuel_flow) while preserving all configuration and logic. The Flow labels remain unchanged as identifiers, which is correct.
61-79: LGTM! CHP parameters correctly updated to new API.The CHP instantiation has been properly updated to use the new parameter names (
thermal_efficiency,electrical_efficiency,electrical_flow,thermal_flow,fuel_flow) while preserving all configuration, on/off parameters, and investment constraints. The Flow labels remain unchanged as identifiers, which is correct.tests/test_scenarios.py (1)
142-170: LGTM! Parameter renames applied correctly.The Boiler constructor has been properly updated to use the new parameter names (
thermal_efficiency,thermal_flow,fuel_flow) with consistent keyword argument style.tests/test_effect.py (1)
250-257: LGTM! Boiler instantiation updated correctly.The test correctly uses the renamed parameters (
thermal_efficiency,thermal_flow,fuel_flow) consistent with the broader API changes.examples/00_Minmal/minimal_example.py (1)
19-24: LGTM! Example updated to use new API.The minimal example correctly demonstrates the new parameter names, providing clear guidance for users migrating to the updated API.
tests/test_component.py (3)
418-423: LGTM! Test updated to use renamed parameters.The Boiler constructor correctly uses the new parameter names throughout this test.
457-469: LGTM! Consistent parameter usage.All Boiler instantiations in this test method properly use the renamed parameters.
534-546: LGTM! Parameter renames applied consistently.Both Boiler instances correctly use the new parameter names.
tests/test_integration.py (3)
40-43: LGTM! Attribute access path updated correctly.The test correctly accesses the
thermal_flowattribute instead of the deprecatedQ_thname.
46-50: LGTM! CHP thermal flow reference updated.Consistent update to use
thermal_flowattribute for the CHP component.
222-226: LGTM! Kessel attribute access updated.The assertion correctly references
thermal_flowconsistent with the API changes.tests/test_flow_system_resample.py (1)
54-58: LGTM! Boiler creation and attribute access updated.Both the constructor call and the subsequent attribute access (
thermal_flow.size) correctly use the new API naming.tests/test_functional.py (2)
165-177: LGTM! Investment attribute path updated correctly.The test correctly accesses investment properties via
thermal_flow.submodel._investment, consistent with the renamed attribute.
357-369: LGTM! On/off attribute paths updated correctly.All on/off related assertions properly reference
thermal_flow.submodel.on_off, consistent with the API changes.examples/01_Simple/simple_example.py (1)
47-62: LGTM! Example comprehensively updated for new API.Both the Boiler and CHP components correctly demonstrate the new parameter names (
thermal_efficiency,electrical_efficiency,thermal_flow,electrical_flow,fuel_flow), providing clear guidance for users.tests/conftest.py (1)
134-143: LGTM! Test fixtures correctly updated to new API.All Boiler and CHP fixture definitions have been successfully updated to use the new parameter names (
thermal_efficiency,thermal_flow,fuel_flow,electrical_flow,electrical_efficiency). The changes are consistent across all fixture variants and maintain the same semantics.Also applies to: 151-152, 178-179, 187-194, 201-206, 599-600, 601-608, 612-617
examples/02_Complex/complex_example.py (2)
51-83: LGTM! Boiler definition correctly updated.The Gaskessel component now uses the new descriptive parameter names (
thermal_efficiency,thermal_flow,fuel_flow), improving code readability.
87-95: LGTM! CHP definition correctly updated.The CHP component now uses the new parameter names (
thermal_efficiency,electrical_efficiency,electrical_flow,thermal_flow,fuel_flow), making the API more intuitive.examples/04_Scenarios/scenario_example.py (1)
115-127: LGTM! Scenario example correctly updated.Both Boiler and CHP components have been updated to use the new parameter names. The changes are consistent with the API updates and maintain the same functionality.
Also applies to: 131-140
examples/03_Calculation_types/example_calculation_types.py (1)
72-84: LGTM! Calculation types example correctly updated.The Boiler and CHP component definitions have been properly updated to use the new descriptive parameter names, ensuring consistency across all examples.
Also applies to: 87-95
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_functional.py (2)
165-165: Consider using the publicinvestmentproperty instead of deprecated_investment.The tests access
boiler.thermal_flow.submodel._investment, but according to the codebase documentation,_investmentis a deprecated alias. Consider updating these references to use the publicinvestmentproperty for better maintainability.Apply this pattern throughout the affected test cases:
- boiler.thermal_flow.submodel._investment.size.solution.item(), + boiler.thermal_flow.submodel.investment.size.solution.item(),- boiler.thermal_flow.submodel._investment.invested.solution.item(), + boiler.thermal_flow.submodel.investment.invested.solution.item(),Also applies to: 172-172, 206-206, 213-213, 247-247, 254-254, 303-303, 310-310, 318-318, 325-325
86-91: Consider adding backward compatibility tests for deprecated parameters.While the tests correctly use the new parameter names (
thermal_efficiency,fuel_flow,thermal_flow), there are no tests verifying that the deprecated parameter names (eta,Q_fu,Q_th) still work and emit proper deprecation warnings. Adding such tests would help ensure the deprecation shims function correctly.Example test to add:
def test_deprecated_boiler_parameters(solver_fixture, time_steps_fixture): """Verify that deprecated Boiler parameters still work with warnings.""" import warnings flow_system = flow_system_base(time_steps_fixture) with warnings.catch_warnings(record=True) as w: warnings.simplefilter("always") flow_system.add_elements( fx.linear_converters.Boiler( 'Boiler', eta=0.5, # deprecated Q_fu=fx.Flow('Q_fu', bus='Gas'), # deprecated Q_th=fx.Flow('Q_th', bus='Fernwärme'), # deprecated ) ) # Verify deprecation warnings were issued assert len(w) == 3 assert all(issubclass(warning.category, DeprecationWarning) for warning in w) # Verify functionality still works results = solve_and_load(flow_system, solver_fixture) assert_allclose(results.model.variables['costs'].solution.values, 80, rtol=1e-5, atol=1e-10)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_functional.py(27 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_functional.py (5)
flixopt/linear_converters.py (7)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)Boiler(24-175)flixopt/elements.py (4)
Flow(294-560)_investment(765-767)on_off(758-762)flow_rate(677-679)flixopt/components.py (1)
_investment(961-963)flixopt/features.py (5)
size(133-135)invested(138-142)off(275-277)switch_on(280-282)switch_off(285-287)flixopt/interface.py (1)
OnOffParameters(1080-1341)
⏰ 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.10)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (2)
tests/test_functional.py (2)
86-91: LGTM! Constructor parameter updates are consistent.All Boiler instantiations have been correctly updated to use the new parameter names (
thermal_efficiency,fuel_flow,thermal_flow). The changes are applied consistently across all test cases.Also applies to: 142-151, 183-193, 224-234, 265-289, 337-343, 376-387, 427-438, 485-502, 535-557, 609-626, 670-688
165-165: LGTM! Attribute access updates are correct.All references to
boiler.Q_thhave been properly updated toboiler.thermal_flow, maintaining consistency with the API changes.Also applies to: 172-172, 206-206, 213-213, 247-247, 254-254, 303-303, 310-310, 318-318, 325-325, 357-357, 364-364, 401-401, 408-409, 415-415, 452-452, 459-459, 466-466, 473-473, 516-516, 523-523, 575-575, 582-582, 590-590, 597-597, 643-643, 650-650, 658-658, 706-706, 713-713, 720-720, 728-728
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
CHANGELOG.md (1)
90-90: Enhance description of thecheck_boundsfix.The description of the fix is brief. Consider adding context about why the normalization improves robustness (e.g., what array-like inputs were problematic before).
Example revision:
- Fixed `check_bounds` function in `linear_converters.py` to use normalized array for comparisons, improving robustness with array-like inputs + Fixed `check_bounds` function in `linear_converters.py` to normalize array inputs before comparisons, ensuring correct boundary checks with DataFrames, Series, and other array-like types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)examples/05_Two-stage-optimization/two_stage_optimization.py(3 hunks)flixopt/components.py(6 hunks)tests/test_scenarios.py(3 hunks)tests/test_storage.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_scenarios.py (1)
flixopt/linear_converters.py (6)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)
examples/05_Two-stage-optimization/two_stage_optimization.py (1)
flixopt/linear_converters.py (8)
thermal_efficiency(115-116)thermal_efficiency(119-121)thermal_efficiency(275-276)thermal_efficiency(279-281)thermal_efficiency(764-765)thermal_efficiency(768-770)electrical_efficiency(773-774)electrical_efficiency(777-779)
flixopt/components.py (1)
flixopt/core.py (1)
PlausibilityError(21-24)
⏰ 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.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
🔇 Additional comments (10)
flixopt/components.py (4)
278-278: LGTM: Docstring updated to reflect the new parameter value.The parameter description correctly documents
'equals_final'as the new sentinel value for cyclic initial charge state.
342-342: LGTM: Example code updated to use the new sentinel value.The docstring example correctly demonstrates the use of
'equals_final'for cyclic storage initialization.
391-391: LGTM: Type hint updated to reflect the new API.The type signature correctly specifies
Literal['equals_final']as the string option for initial charge state.
511-522: LGTM: Conditional logic correctly guards numeric comparisons.The logic properly skips numeric bound checks when
initial_charge_state='equals_final', preventing type errors when comparing string literals to numeric bounds.tests/test_scenarios.py (2)
88-88: LGTM: Test updated to use the new sentinel value.The test correctly uses
'equals_final'instead of the deprecated value, aligning with the API changes inflixopt/components.py.
140-170: LGTM: Boiler parameters updated to new API.The test correctly uses the renamed parameters:
eta→thermal_efficiencyQ_th→thermal_flowQ_fu→fuel_flowThese changes align with the parameter renaming in linear converters.
tests/test_storage.py (1)
365-365: LGTM: Test updated to use the new sentinel value.The cyclic initialization test correctly uses
'equals_final'instead of the deprecated value, ensuring test coverage for the renamed API.examples/05_Two-stage-optimization/two_stage_optimization.py (3)
46-60: LGTM: Boiler parameters updated to new API.The example correctly uses the renamed parameters:
eta→thermal_efficiencyQ_th→thermal_flowQ_fu→fuel_flow
61-79: LGTM: CHP parameters updated to new API.The example correctly uses the renamed parameters:
eta_th→thermal_efficiencyeta_el→electrical_efficiencyP_el→electrical_flowQ_th→thermal_flowQ_fu→fuel_flow
85-85: LGTM: Storage parameter updated to new API.The example correctly uses
'equals_final'instead of the deprecated value for cyclic initialization.
CHANGELOG.md
Outdated
| - **Parameter renaming in `linear_converters.py`**: Renamed parameters to use lowercase, descriptive names for better consistency and clarity: | ||
| - **Flow parameters** (deprecated uppercase abbreviations → descriptive names): | ||
| - `Boiler`: `Q_fu` → `fuel_flow`, `Q_th` → `thermal_flow` | ||
| - `Power2Heat`: `P_el` → `power_flow` → `electrical_flow`, `Q_th` → `thermal_flow` |
There was a problem hiding this comment.
Clarify deprecation path for power_flow parameter.
The power_flow parameter treatment is inconsistent between sections. Line 66 shows a two-step migration (P_el → power_flow → electrical_flow), but line 82 lists power_flow alongside parameters that should be replaced with electrical_flow. This may confuse users about the correct migration path.
If power_flow is an intermediate step, the Deprecated section should clarify whether it's also deprecated or only a transitional name. Alternatively, if all three names are equally deprecated, the Changed section should be clearer.
Consider revising line 82 to clarify the deprecation strategy:
- **Flow parameters**: `Q_fu`, `Q_th`, `P_el`, `Q_ab`, `power_flow` (use `fuel_flow`, `thermal_flow`, `electrical_flow`, `heat_source_flow` instead)
+ **Flow parameters**: `Q_fu`, `Q_th`, `P_el`, `Q_ab` (use `fuel_flow`, `thermal_flow`, `electrical_flow`, `heat_source_flow` instead). Note: `power_flow` is a transitional name for `P_el` and should migrate to `electrical_flow`.Also applies to: 82-82
🤖 Prompt for AI Agents
CHANGELOG.md around lines 66 and 82: the changelog shows inconsistent
migration/deprecation guidance for the parameter names (line 66 shows P_el →
power_flow → electrical_flow, while line 82 treats power_flow as if it should be
replaced directly with electrical_flow), which can confuse users; update the
entries to state a single clear deprecation path: either mark power_flow as an
intermediate/transitional name and note its deprecation with exact replacement
(e.g., "P_el -> power_flow (transitional, deprecated) -> electrical_flow"), or
declare all three names deprecated and list their canonical replacement; revise
both line 66 and line 82 contents so they use the same wording and explicitly
state whether power_flow is transitional or fully deprecated, and add a short
parenthetical note indicating the canonical name users should adopt going
forward.
…-converters' into feature/rename-parameters-linear-converters
…ters-linear-converters # Conflicts: # CHANGELOG.md
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Changed
Deprecated
Bug Fixes
Documentation