Change default size to be None. If mathematically needed, Users need to specify it #517
Conversation
WalkthroughRefactors sizing semantics: Flow size may be None (unbounded) instead of defaulting to a large constant; InvestParameters accepts an explicit optional maximum_size and now requires either fixed_size or maximum_size when used for investment scaling. Additional plausibility checks enforce size presence when status parameters, fixed_relative_profile, relative bounds, or load_factors depend on size. Tests updated to provide explicit sizes/maximum_size values. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
flixopt/elements.py(5 hunks)flixopt/interface.py(3 hunks)flixopt/io.py(1 hunks)tests/deprecated/test_component.py(2 hunks)tests/deprecated/test_effect.py(1 hunks)tests/deprecated/test_functional.py(4 hunks)tests/deprecated/test_storage.py(1 hunks)tests/test_component.py(2 hunks)tests/test_effect.py(1 hunks)tests/test_functional.py(4 hunks)tests/test_storage.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/deprecated/test_effect.py (1)
flixopt/interface.py (1)
InvestParameters(693-1017)
tests/test_functional.py (2)
flixopt/features.py (1)
size(140-142)flixopt/interface.py (1)
InvestParameters(693-1017)
tests/deprecated/test_component.py (2)
flixopt/elements.py (1)
Flow(319-603)flixopt/interface.py (1)
InvestParameters(693-1017)
tests/test_effect.py (1)
flixopt/interface.py (1)
InvestParameters(693-1017)
🪛 GitHub Actions: Tests
tests/test_component.py
[error] 594-594: AssertionError: Flow 'Wärme' status does not match expected. Max difference is 1, indicating missing or incorrectly set status variables.
[error] 590-590: AssertionError: Flow 'Wärme' status does not match expected. Max difference is 1, indicating missing or incorrectly set status variables.
[error] 511-511: AssertionError: Flow 'Wärme' status does not match expected. Max difference is 1, indicating missing or incorrectly set status variables.
flixopt/elements.py
[error] 556-556: Flow 'BHKW2(Q_th)' has status_parameters but no size defined. A size is required when using status_parameters to bound the flow rate.
[error] 556-556: Flow 'Sink(Wärme)' has status_parameters but no size defined. A size is required when using status_parameters to bound the flow rate.
[error] 556-556: Flow 'KWK(Q_fu)' has status_parameters but no size defined. A size is required when using status_parameters to bound the flow rate.
[error] 556-556: Flow 'KWK(Q_th)' has status_parameters but no size defined. A size is required when using status_parameters to bound the flow rate.
[error] 556-556: Flow 'Q_th' has status_parameters but no size defined. A size is required when using status_parameters to bound the flow rate.
🔇 Additional comments (25)
flixopt/io.py (1)
804-804: Documentation update correctly reflects deprecation.The updated docstring clearly indicates the parameter is deprecated and that
size=Noneis now the default behavior, aligning with the broader refactoring in this PR.flixopt/interface.py (3)
721-722: Documentation accurately reflects the new requirement.The updated docstring clearly communicates that
maximum_sizeis required whenfixed_sizeis not set, which aligns with the validation logic intransform_data.
904-904: Clean assignment without implicit defaults.Storing
maximum_sizedirectly without applying a default is appropriate since validation is now handled intransform_data, where thename_prefixcontext is available for clearer error messages.
914-920: Well-implemented validation with clear error messaging.The validation correctly enforces that at least one upper bound (
fixed_sizeormaximum_size) must be set. The error message is informative, includes thename_prefixfor easy identification, and explains why an upper bound is necessary for optimization scaling.tests/deprecated/test_storage.py (1)
454-454: Test correctly updated to include requiredmaximum_size.The addition of
maximum_size=100satisfies the newInvestParametersvalidation requirement and maintains consistency with the existing test assertions that verify upper bound constraints.tests/deprecated/test_component.py (2)
501-501: Test correctly updated with explicitmaximum_size.The
maximum_size=1000value is consistent with other flow sizes in this transmission test scenario and satisfies the new validation requirement.
581-583: Test correctly updated with completeInvestParametersspecification.The multi-line format improves readability while adding the required
maximum_size=1000. This maintains consistency with the test's assertion that the investment size equals the minimum of 10 (line 617).tests/test_storage.py (1)
454-454: Test correctly updated to include requiredmaximum_size.This change mirrors the update in
tests/deprecated/test_storage.py, maintaining consistency across test suites while satisfying the newInvestParametersvalidation requirement.tests/test_functional.py (4)
188-188: Test correctly updated withmaximum_sizethat doesn't affect optimal solution.The
maximum_size=100is appropriately larger than the expected optimal size of 20, ensuring the test continues to validate that the optimizer correctly determines the minimum required investment size.
227-229: Test correctly specifies both size bounds.The multi-line format clearly shows both
minimum_size=40andmaximum_size=100, with the expected solution (size=40) being at the lower bound, validating that constraints are properly enforced.
269-274: Test correctly updated for optional investment with bounded size.The first Boiler's
InvestParametersnow includesmaximum_size=100, satisfying the validation requirement while preserving the test's purpose of verifying that the optimizer selects the cheaper optional investment.
285-289: Test correctly updated for the optional investment that should be skipped.The second Boiler's
InvestParametersincludesmaximum_size=100and a higherminimum_size=50, which correctly results in this investment being skipped (size=0) since the first Boiler at minimum_size=40 is the more cost-effective option.tests/deprecated/test_effect.py (1)
256-258: Test correctly updated with completeInvestParametersspecification.The multi-line format improves readability while adding
maximum_size=200. Combined withmandatory=True, this ensures the investment occurs, enabling the effect share calculations that are the focus of this test.tests/test_effect.py (1)
254-256: LGTM! Maximum size bound added to investment parameters.The change adds
maximum_size=200to align with the new InvestParameters API requirement that eitherfixed_sizeormaximum_sizemust be set. The test now properly bounds the thermal flow investment decision.tests/test_component.py (3)
501-501: LGTM! Maximum size bound added for transmission flow.The change adds
maximum_size=1000to satisfy the new requirement that InvestParameters must have eitherfixed_sizeormaximum_sizeset. Previously empty InvestParameters would now fail validation.
577-579: LGTM! Upper bound added to investment parameters.The addition of
maximum_size=1000completes the investment parameter specification, providing an upper bound alongside the existingminimum_size=10and cost parameters.
459-610: The review comment references pipeline test failures at lines 511, 590, and 594 with the message "Flow 'Wärme' status does not match expected." However, examination of the actual test code shows:
- The assertions at those line locations reference
'Rohr(Rohr1a)|status'and'Rohr(Rohr2a)|status', not any 'Wärme' flow status variable- 'Wärme' appears only as bus names ('Wärme lokal') or as flow destination buses, never as a flow variable name with status assertions
- The basic_flow_system fixture adds buses via
Buses.defaults()but does not define any 'Wärme' flow that would have a status variable- No pipeline output files or error logs are present in the repository to corroborate the claimed failures
The original review appears to be based on inaccurate error information or misidentified line numbers. The actual code does not contain assertions about a 'Wärme' flow status, and the test structure is consistent with the assertions shown.
Likely an incorrect or invalid review comment.
tests/deprecated/test_functional.py (3)
190-190: LGTM! Upper bound added to investment parameters.The addition of
maximum_size=100satisfies the new API requirement while maintaining the test's intent to optimize investment size. The test expects the optimizer to choose size=20 (line 206), well within the maximum.
231-233: LGTM! Investment parameters properly bounded.The reformatted InvestParameters now includes
maximum_size=100alongside the existingminimum_size=40. The test expects size=40 to be chosen (line 249), which aligns with minimum cost strategy within the bounded range.
274-296: LGTM! Optional investment parameters properly configured.Both boilers now have properly bounded investment parameters with
maximum_size=100. The test correctly expects:
- First boiler (mandatory): size=40 (minimum), invested=1
- Second boiler (optional): size=0, invested=0
The reformatting improves readability while maintaining test intent.
flixopt/elements.py (5)
344-344: LGTM! Documentation correctly reflects unbounded size handling.The documentation clearly states that
size=Nonecreates an unbounded flow and identifies the specific cases where size is required (status_parameters and fixed_relative_profile). This aligns with the implementation changes.Also applies to: 445-447
477-477: LGTM! Size stored without defaults.The change removes automatic defaulting for
Nonesizes, allowing unbounded flows. This is a cleaner approach that makes size handling explicit.
544-547: LGTM! Coordinate transformation correctly handles None size.The conditional logic properly handles three cases:
- InvestParameters (lines 544-545): delegates transformation to the parameter object
- Numeric size (lines 546-547): transforms coordinates
- None size (implicit): skips transformation, preserving unbounded state
554-565: LGTM! Critical validation checks correctly enforce size requirements.The validation correctly requires size when using status_parameters or fixed_relative_profile, as these features fundamentally depend on bounded flow rates. The error messages clearly explain why size is required:
- status_parameters: needed for big-M constraints
- fixed_relative_profile: needed for the calculation flow_rate = size × profile
These are proper breaking changes that prevent subtle modeling errors.
Note: Pipeline failures confirm these validations are working as intended. The failing tests need to be updated to provide size parameters.
819-843: LGTM! Bounds logic correctly handles unbounded flows.The updated logic properly handles all size scenarios:
- Lower bound (lines 826-834): Applies relative minimum only when size is defined; defaults to 0
- Upper bound (lines 836-841):
- Investment case: uses
maximum_or_fixed_size- Fixed size case: scales by
size- Unbounded case (size=None): returns
np.infThis ensures optimization variables are properly bounded or explicitly unbounded.
…ly through component status
… into feature/solution-storage-change+default-size # Conflicts: # flixopt/elements.py # flixopt/interface.py
5. Component with status_parameters now validates that all flows have explicit sizes
- relative_minimum > 0 requires size (lines 585-590) - relative_maximum < 1 requires size (lines 592-596) - load_factor_min requires size (lines 598-603) - load_factor_max requires size (lines 605-609)
…storage-change+default-size
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/conftest.py (1)
619-622: Optional: reduce “magic number” sizing in flow_system_long for readability.
size=288 * 0.22/288 * 0.58is probably correct, but consider introducing named locals (e.g.,fuel_size,eta_el,eta_th) to make intent obvious and avoid future drift.flixopt/elements.py (1)
471-476: Type hint mismatch:sizedefault is None but annotation omits| None.- size: Numeric_PS | InvestParameters = None, + size: Numeric_PS | InvestParameters | None = None,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
flixopt/components.py(7 hunks)flixopt/elements.py(6 hunks)flixopt/interface.py(3 hunks)flixopt/io.py(1 hunks)tests/conftest.py(3 hunks)tests/deprecated/conftest.py(3 hunks)tests/deprecated/test_component.py(5 hunks)tests/deprecated/test_flow.py(1 hunks)tests/deprecated/test_functional.py(4 hunks)tests/deprecated/test_scenarios.py(1 hunks)tests/test_component.py(5 hunks)tests/test_flow.py(1 hunks)tests/test_functional.py(4 hunks)tests/test_scenarios.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- flixopt/io.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_functional.py
🧰 Additional context used
🧬 Code graph analysis (8)
tests/conftest.py (1)
flixopt/elements.py (1)
Flow(333-647)
tests/test_flow.py (2)
flixopt/results.py (1)
size(2022-2030)flixopt/features.py (1)
size(140-142)
tests/deprecated/test_scenarios.py (1)
flixopt/elements.py (1)
Flow(333-647)
tests/deprecated/test_functional.py (1)
flixopt/interface.py (1)
InvestParameters(857-1180)
tests/test_scenarios.py (3)
flixopt/elements.py (1)
Flow(333-647)flixopt/results.py (1)
size(2022-2030)flixopt/features.py (1)
size(140-142)
tests/deprecated/test_component.py (2)
flixopt/elements.py (1)
Flow(333-647)flixopt/interface.py (1)
InvestParameters(857-1180)
flixopt/elements.py (4)
flixopt/features.py (1)
size(140-142)flixopt/core.py (1)
PlausibilityError(22-25)flixopt/structure.py (2)
label_full(1100-1101)label_full(1646-1647)flixopt/interface.py (2)
minimum_or_fixed_size(1143-1144)maximum_or_fixed_size(1147-1148)
flixopt/components.py (2)
flixopt/interface.py (3)
InvestParameters(857-1180)minimum_or_fixed_size(1143-1144)maximum_or_fixed_size(1147-1148)flixopt/core.py (1)
PlausibilityError(22-25)
🔇 Additional comments (27)
flixopt/interface.py (3)
881-887: Docstring now matches the new “explicit maximum_size” semantics.
This makes the constraint more discoverable for users.
1067-1069: Good: maximum_size no longer silently defaults to CONFIG.Modeling.big.
This avoids surprising “huge bound” behavior and aligns with “size=None can be unbounded” elsewhere.
1078-1083: No action needed—_fit_coordssafely handlesNonevalues.
fit_to_model_coords()(which_fit_coordsdelegates to) has an explicit None check at line 927-928 inflixopt/flow_system.pythat returnsNoneimmediately. Whenfixed_sizeis set andmaximum_sizeisNone, the call toself._fit_coords(..., self.maximum_size, ...)will returnNonewithout error. The code is correct as written.tests/test_scenarios.py (1)
212-219: Good test hardening: explicit sizes for KWK piecewise flows.
This aligns the scenario fixture with the new sizing semantics and avoids size-dependent plausibility errors.tests/conftest.py (1)
214-220: Nice: fixtures now set sizes where the model needs them (piecewise/segments converters).
These sizes line up cleanly with the piecewise conversion ranges and should prevent size-related plausibility failures.Also applies to: 234-240
tests/deprecated/test_flow.py (1)
593-600: Good: status-based effects test now provides a size.
This keeps the test compatible with the “size required when using status_parameters” rule.tests/test_flow.py (1)
593-600: Good: status-based effects test now provides a size.
This aligns with the updated sizing/plausibility rules.tests/deprecated/test_component.py (1)
34-40: Good: component test flows now specify size alongside relative_minimum.
This matches the new requirement that relative bounds are only meaningful with a defined size.tests/deprecated/test_scenarios.py (1)
212-219: Good: deprecated scenario fixture updated with explicit sizes for piecewise KWK flows.
Keeps deprecated tests aligned with the new sizing semantics.tests/deprecated/conftest.py (3)
219-223: Good: explicit sizes for piecewise converter flows match piecewise domains (and avoid size=None pitfalls).
239-243: Good: explicit sizes for segments converter inputs/outputs keep the fixture compatible with new plausibility rules.
619-620: Good: CHP BHKW2 now has explicit P_el/Q_th sizes (prevents status-related “missing size” errors).tests/deprecated/test_functional.py (3)
189-190: LGTM: adds explicit maximum_size to keep InvestParameters valid under new sizing semantics.
230-233: LGTM: min/max sizing test now sets maximum_size explicitly (aligns with new InvestParameters contract).
274-279: LGTM: optional investment tests now provide maximum_size bounds for both variants.Also applies to: 290-295
tests/test_component.py (4)
34-40: Good: explicit sizes on test component flows prevent new size-dependent plausibility errors.
467-470: Good: Boiler_Standard now has size for time-varying relative_maximum bounds (required under new rules).Also applies to: 542-545
503-504: Looks right: balanced transmission now givesin2an InvestParameters(maximum_size=...) so “same_size” can be enforced.
581-584: Good: unbalanced transmission now fully specifies InvestParameters bounds/costs onin2.flixopt/elements.py (6)
141-150: Good guardrail: component-level status_parameters now enforces explicit sizes on all connected flows.
358-361: Docs are clearer: “size=None means unbounded” + explicit “size required” cases are called out.Also applies to: 459-461
491-491: LGTM: storing the provided size directly supports the new “unbounded by default” semantics.
564-566: LGTM: only fitting coords for scalar size (and transforming InvestParameters) matches expected data-shaping behavior.
572-609: Good: size-dependent constraints now fail fast with targeted PlausibilityError messages.
874-886: No change needed—np.infis correct for linopy unbounded variables. Linopy usesnp.inf(and-np.inf) to represent unbounded variable bounds;Noneis not a valid unbounded marker for variable bounds in linopy. The current code is correct.flixopt/components.py (2)
495-517: Good: capacity=None is now explicitly rejected when relative (non-default) charge-state bounds depend on capacity.
971-985: linopy supportsnp.inffor variable bounds as the standard way to represent unbounded constraints. The current implementation returning(0, np.inf)is correct and aligns with linopy's documented behavior. No change needed.
flixopt/components.py
Outdated
| # Initial capacity should not constraint investment decision | ||
| minimum_initial_capacity = maximum_capacity * self.relative_minimum_charge_state.isel(time=0) | ||
| maximum_initial_capacity = minimum_capacity * self.relative_maximum_charge_state.isel(time=0) | ||
|
|
||
| # Only perform numeric comparisons if not using 'equals_final' | ||
| if not initial_equals_final: | ||
| if (self.initial_charge_state > maximum_initial_capacity).any(): | ||
| raise PlausibilityError( | ||
| f'{self.label_full}: {self.initial_charge_state=} ' | ||
| f'is constraining the investment decision. Chosse a value above {maximum_initial_capacity}' | ||
| ) | ||
| if (self.initial_charge_state < minimum_initial_capacity).any(): | ||
| raise PlausibilityError( | ||
| f'{self.label_full}: {self.initial_charge_state=} ' | ||
| f'is constraining the investment decision. Chosse a value below {minimum_initial_capacity}' | ||
| ) |
There was a problem hiding this comment.
Bug: initial_charge_state feasibility checks use inverted min/max capacity multipliers (will raise incorrectly).
- minimum_initial_capacity = maximum_capacity * self.relative_minimum_charge_state.isel(time=0)
- maximum_initial_capacity = minimum_capacity * self.relative_maximum_charge_state.isel(time=0)
+ min_initial_allowed = minimum_capacity * self.relative_minimum_charge_state.isel(time=0)
+ max_initial_allowed = maximum_capacity * self.relative_maximum_charge_state.isel(time=0)
# Only perform numeric comparisons if not using 'equals_final'
if not initial_equals_final:
- if (self.initial_charge_state > maximum_initial_capacity).any():
+ if (self.initial_charge_state > max_initial_allowed).any():
raise PlausibilityError(
f'{self.label_full}: {self.initial_charge_state=} '
- f'is constraining the investment decision. Chosse a value above {maximum_initial_capacity}'
+ f'is infeasible w.r.t. the maximum capacity. Choose a value <= {max_initial_allowed}.'
)
- if (self.initial_charge_state < minimum_initial_capacity).any():
+ if (self.initial_charge_state < min_initial_allowed).any():
raise PlausibilityError(
f'{self.label_full}: {self.initial_charge_state=} '
- f'is constraining the investment decision. Chosse a value below {minimum_initial_capacity}'
+ f'is infeasible w.r.t. the minimum capacity. Choose a value >= {min_initial_allowed}.'
)🤖 Prompt for AI Agents
In flixopt/components.py around lines 528 to 543 the computation of initial
capacity bounds is inverted: minimum_initial_capacity is calculated from
maximum_capacity and maximum_initial_capacity from minimum_capacity, causing
valid initial_charge_state values to be flagged as infeasible; change
minimum_initial_capacity to use minimum_capacity *
self.relative_minimum_charge_state.isel(time=0) and maximum_initial_capacity to
use maximum_capacity * self.relative_maximum_charge_state.isel(time=0), keep the
subsequent comparisons the same so that values above maximum_initial_capacity
and below minimum_initial_capacity raise PlausibilityError with the existing
messages.
…storage-change+default-size
…storage-change+default-size
…storage-change+default-size
…storage-change+default-size
…storage-change+default-size
… into feature/solution-storage-change+default-size
FBumann
changed the title
FBumann
changed the title
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.