Feature/vintage investment timing #502
Conversation
…. Here's a summary of what was implemented: Summary of Changes 1. core.py - Added 'vintage' to FlowSystemDimensions - Updated FlowSystemDimensions type to include 'vintage' 2. types.py - Added vintage type aliases - Numeric_VS - Vintage, Scenario dimensions - Numeric_PVS - Period, Vintage, Scenario dimensions - Numeric_TPVS - Time, Period, Vintage, Scenario dimensions - Bool_VS, Bool_PVS - Boolean variants - Effect_VS, Effect_PVS - Effect dict variants 3. flow_system.py - Added vintages and vintage_weights - vintages property derived from periods (same values, different name) - vintage_weights defaults to 1.0 for all vintages (one-time costs not scaled) - all_coords property that includes vintage 4. structure.py - Added vintage_weights property - vintage_weights property on FlowSystemModel - Updated get_coords to work with vintage dimension when explicitly requested 5. effects.py - Major updates - Added vintage parameters: vintage_weights, share_from_vintage, minimum_vintage, maximum_vintage, minimum_over_vintages, maximum_over_vintages, minimum_overall, maximum_overall - Added minimum_total/maximum_total for per-period total bounds - Added vintage ShareAllocationModel (only when periods are defined) - Updated objective function to weight temporal/periodic and vintage separately - Added vintage target to add_share_to_effects - Added vintage cross-effect shares in _add_share_between_effects - Kept total variable for backwards compatibility (temporal + periodic only) The remaining tasks (SizingParameters and InvestmentParameters) are separate from the vintage dimension work and would require further implementation to split the current InvestParameters into distinct sizing vs investment classes with the active matrix logic.
|
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 pull request introduces comprehensive vintage dimension support throughout the flixopt modeling framework. It adds new SizingParameters and InvestmentParameters classes with a shared _SizeModel base, extends effects with vintage-aware constraints and sharing, updates the type system with vintage-related aliases, and adds corresponding documentation and test updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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 |
|
@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: 3
🧹 Nitpick comments (4)
flixopt/elements.py (2)
815-829: Lower bound logic for mandatory investments looks sound but assumes array-likemandatoryUsing
self.element.size.mandatory.all()so that the minimum-relative bound is only enforced when all mandatory flags are true avoids over-constraining partially-optional investments and is consistent with scenario/period arrays. This relies onmandatoryalways being an array-like (e.g. xarray/NumPy) rather than a plain bool; if plain booleans are still possible, consider normalizing them earlier or wrapping withbool(np.asarray(self.element.size.mandatory).all())to avoid surprises.
451-455:with_investmentonly checksInvestParametersdespite new sizing classes
Flow.sizeis typed asNumeric_PS | InvestParameters, andwith_investmentis implemented asisinstance(self.element.size, InvestParameters). With the introduction ofSizingParametersandInvestmentParameters(andInvestParametersnow being a deprecation wrapper), flows configured directly with the new classes may not be treated as “with investment” unless those classes subclassInvestParameters.It’s worth double-checking the hierarchy and, if needed, broadening the check, for example:
-from .interface import InvestParameters, StatusParameters +from .interface import InvestParameters, InvestmentParameters, SizingParameters, StatusParameters ... @property def with_investment(self) -> bool: - return isinstance(self.element.size, InvestParameters) + return isinstance( + self.element.size, + (InvestParameters, SizingParameters, InvestmentParameters), + )and optionally updating the type hint on
sizeaccordingly. Based on relevant code snippets fromflixopt.interface, whereSizingParametersandInvestmentParametersare the new primary APIs.Also applies to: 747-748
flixopt/flow_system.py (1)
180-207: Vintages, vintage weights, and coordinate helpers integrate cleanly
- Initializing
self.vintagesasperiods.rename('vintage')and defaultingself.vintage_weightsto ones over that index gives a clear semantics: vintages mirror periods but are used specifically for one‑time investment decisions.- When no periods are defined, both
vintagesandvintage_weightsremainNone, so vintage-aware code can short‑circuit cleanly.- The updated
coords/all_coordssplit—coordsfor the traditional (time, period, scenario) set andall_coordsaddingvintage—is consistent with the new docstring comments and the FlowSystemModel.get_coords implementation.If you later allow user-defined vintage weights via the public API, you may want to thread them through
__init__and serialization similarly toscenario_weights, but for the current behavior this wiring is consistent.Also applies to: 1088-1111
flixopt/interface.py (1)
1036-1040: Warning for lifetime exceeding horizon is helpful.The warning when
lifetime > max_horizonhelps users understand potential issues. However, the warning message could be improved for clarity.Consider slightly improving the warning message:
if (self.lifetime > max_horizon).any(): logger.warning( - f'Fixed lifetime ({self.lifetime.values}) if Investment exceeds model horizon ({max_horizon}). ' + f'Lifetime ({self.lifetime.values}) exceeds model horizon ({max_horizon}). ' + f'Investment in late periods may extend beyond the optimization timeframe.' )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/user-guide/mathematical-notation/features/InvestmentParameters.md(1 hunks)docs/user-guide/mathematical-notation/features/SizingParameters.md(7 hunks)flixopt/__init__.py(2 hunks)flixopt/core.py(1 hunks)flixopt/effects.py(13 hunks)flixopt/elements.py(1 hunks)flixopt/features.py(3 hunks)flixopt/flow_system.py(3 hunks)flixopt/interface.py(2 hunks)flixopt/structure.py(3 hunks)flixopt/types.py(4 hunks)tests/test_effect.py(5 hunks)tests/test_scenarios.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
flixopt/flow_system.py (2)
flixopt/effects.py (1)
vintage_weights(431-447)flixopt/structure.py (1)
vintage_weights(218-233)
tests/test_scenarios.py (2)
flixopt/effects.py (3)
objective_effect(768-771)objective_effect(774-783)penalty_effect(786-805)flixopt/results.py (1)
objective(300-307)
tests/test_effect.py (2)
flixopt/structure.py (8)
flow_system(350-368)add(1087-1095)add(1620-1622)variables(1520-1526)constraints(1511-1517)add_variables(1407-1416)get_coords(245-277)get_coords(1466-1471)flixopt/flow_system.py (1)
coords(1088-1099)
flixopt/features.py (7)
flixopt/interface.py (3)
InvestmentParameters(885-1040)SizingParameters(792-848)StatusParameters(1044-1295)flixopt/structure.py (11)
Submodel(1383-1562)add_variables(1407-1416)get_coords(245-277)get_coords(1466-1471)add_constraints(1418-1427)get(1459-1464)get(1624-1626)_do_modeling(1555-1562)flow_system(350-368)label_full(1005-1006)label_full(1497-1498)flixopt/flow_system.py (2)
coords(1088-1099)sel(1284-1312)flixopt/modeling.py (4)
BoundingPatterns(398-772)bounds_with_state(434-476)state_transition_bounds(575-626)link_changes_to_level_with_binaries(698-772)tests/test_io.py (1)
flow_system(27-32)tests/test_results_plots.py (1)
flow_system(15-16)tests/test_network_app.py (1)
flow_system(13-18)
flixopt/interface.py (1)
flixopt/structure.py (9)
Interface(300-977)_set_flow_system(334-347)flow_system(350-368)transform_data(318-332)_fit_effect_coords(385-403)_fit_coords(370-383)_plausibility_checks(996-999)values(1330-1335)values(1617-1618)
flixopt/structure.py (2)
flixopt/effects.py (1)
vintage_weights(431-447)flixopt/flow_system.py (2)
coords(1088-1099)all_coords(1102-1111)
flixopt/__init__.py (1)
flixopt/interface.py (4)
InvestmentParameters(885-1040)InvestParameters(852-875)SizingParameters(792-848)StatusParameters(1044-1295)
🪛 LanguageTool
docs/user-guide/mathematical-notation/features/InvestmentParameters.md
[style] ~4-~4: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 2099 characters long)
Context: ...del WHEN to invest, not just how much. !!! info "Relationship to SizingParameters"...
(EN_EXCESSIVE_EXCLAMATION)
docs/user-guide/mathematical-notation/features/SizingParameters.md
[style] ~4-~4: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1381 characters long)
Context: ...iable — should we build this? How big? !!! note "Naming Change" `SizingParamet...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
docs/user-guide/mathematical-notation/features/InvestmentParameters.md
47-47: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
63-63: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
77-77: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
94-94: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
111-111: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
docs/user-guide/mathematical-notation/features/SizingParameters.md
34-34: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
57-57: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
86-86: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
98-98: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
⏰ 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.11)
- GitHub Check: test (3.10)
🔇 Additional comments (30)
flixopt/core.py (1)
18-18: FlowSystemDimensions now correctly covers the newvintageaxisIncluding
'vintage'inFlowSystemDimensionsmatches the newFlowSystem.all_coords/vintagesusage and keeps typing aligned with the extended coordinate set.docs/user-guide/mathematical-notation/features/SizingParameters.md (1)
1-147: SizingParameters docs align with the renamed APIThe examples and symbol table consistently use
SizingParameterswitheffects_per_size,effects_of_size,effects_of_retirement, andpiecewise_effects_per_size, which matches the new sizing terminology and expected constructor fields.flixopt/structure.py (1)
217-234: Vintage weights and coord selection behavior are coherent with FlowSystem’s newvintageaxis
FlowSystemModel.vintage_weightscorrectly:
- returns scalar
1when the FlowSystem has no vintages,- builds a 1‑D
('vintage',)array of ones when vintages exist but no weights are provided, and- defers to
flow_system.vintage_weightsotherwise.get_coordsnow:
- keeps the previous default (
coordswithoutvintagewhendims is None),- uses
flow_system.all_coordswhendimsis specified so callers can explicitly requestvintage,- and guards
extra_timestep=Truewhentimeis omitted.This matches the FlowSystem-side changes and offers an explicit opt‑in for vintage in modeling code.
Also applies to: 245-273
tests/test_scenarios.py (1)
241-264: Objective checks now correctly include temporal and periodic parts for both cost and penaltyThe updated
expected_objintest_weightsandtest_weights_iosums:
costs_submodel.temporal.total× scenario weights,costs_submodel.periodic.total× scenario weights,penalty_submodel.temporal.total× scenario weights,penalty_submodel.periodic.total× scenario weights,and compares this against
model.objective.expressionwithassert_lineequal. This matches the decomposition of the objective into temporal and periodic contributions for both the objective and penalty effects and is consistent with howobjective_weightsare validated earlier in each test.Also applies to: 267-291
flixopt/__init__.py (1)
31-40: Top-level exports now expose new sizing/investment parameter classesRe-exporting
InvestmentParametersandSizingParametersfrom.interfaceand adding them to__all__(alongside the existingInvestParameters) makes the new APIs available asflixopt.InvestmentParameters/flixopt.SizingParameterswhile keeping backward compatibility. This is consistent with the documentation updates and test imports.Also applies to: 43-73
tests/test_effect.py (3)
25-50: Well-structured conditional vintage expectations.The dynamic construction of expected variables and constraints based on
flow_system.vintagespresence is clean and maintainable. This pattern correctly adapts tests to both vintage-enabled and non-vintage configurations.
85-89: Good coverage of total constraint validation.The explicit assertion that
total = temporal + periodicaligns with the implementation ineffects.pywhere the total constraint excludes vintage (which has a different dimension).
200-232: Cross-effect share testing is comprehensive.The test correctly verifies the expected variables and constraints for cross-effect sharing, including the conditional vintage handling. The mapping pattern
Effect1(periodic)->Effect2(periodic)is clearly tested.flixopt/types.py (3)
6-14: Documentation table clearly explains dimension suffixes.The updated table with
_TPVS,_PVS, and_VSsuffixes is helpful for understanding the vintage dimension semantics.
74-83: Vintage-related numeric types are well-documented.The docstrings clearly explain each type's purpose:
Numeric_VS: One-time investment parametersNumeric_PVS: Vintage-dependent parameters varying by periodNumeric_TPVS: Time-varying vintage-dependent data
140-154: Transformed data types provide useful abstractions.
PeriodicData,PeriodicEffects, andPeriodicEffectsUserclearly distinguish between user input formats and internal transformed representations.flixopt/interface.py (5)
695-719: Clean base class extraction for sizing parameters.The
_SizeParametersclass properly centralizes common sizing logic including effects, bounds, and piecewise handling. The default values fromCONFIG.Modelingare appropriate.
759-766: Helpful computed properties for bounds resolution.
minimum_or_fixed_sizeandmaximum_or_fixed_sizeprovide clean abstractions for accessing the effective size bounds regardless of whetherfixed_sizeis set.
851-876: Good deprecation strategy for backwards compatibility.The
InvestParameterswrapper correctly maps old parameter names to new ones and emits a clear deprecation warning with migration guidance. Thestacklevel=2ensures the warning points to user code.
968-970: Validate lifetime parameter early.The explicit check for
Nonelifetime is good, but consider if a more descriptive error message would help users understand what lifetime means.
1028-1031: The force_investment validation logic is correct.The check
(self.force_investment.sum('period') > 1).any()correctly validates thatforce_investmentcan only be True for a single period per scenario. After_fit_coords, the DataConverter explicitly preserves boolean dtype, so the sum operation correctly treats True as 1 and False as 0. This catches violations where a scenario has multiple forced investment periods.flixopt/features.py (5)
29-51: Good defensive type checking for mandatory parameter.The explicit dtype check ensures
mandatoryis boolean, preventing subtle bugs from numeric values being interpreted incorrectly.
59-79: Clear logic for optional vs mandatory sizing.The binary
investedvariable is only created when sizing is optional (not mandatory.all()), avoiding unnecessary binary variables when sizing is always required. The constraintinvested.where(mandatory) == 1correctly forces investment where mandatory.
100-113: Retirement effects formula has correct semantics.The formula
-invested * factor + factor = factor * (1 - invested)correctly applies retirement effects only wheninvested=0. The guardnot is_all_mandatoryprevents adding these effects when retirement is impossible.
311-329: Investment constraint logic handles force_investment correctly.The check for
force_investment.sum('period') > 1before adding the constraint ensures consistency with the plausibility check inInvestmentParameters._plausibility_checks().
338-363: Investment-timing effects use dimension renaming correctly.Renaming
periodtoperiod_of_investmentbefore summing correctly aggregates effects across potential investment periods. This preserves the original period dimension for the result.flixopt/effects.py (9)
58-91: Comprehensive vintage parameter documentation.The docstring clearly explains the new vintage-related parameters including
vintage_weights,share_from_vintage, and the various vintage bounds. The distinction betweenminimum_over_periodsandminimum_over_vintagesis well-documented.
369-386: Plausibility checks correctly validate dimension requirements.The checks ensure that vintage-related constraints (
minimum_over_vintages,maximum_over_vintages) require the vintage dimension, and overall constraints require the period dimension. This prevents runtime errors from misconfigurations.
430-448: Vintage weights fallback logic mirrors period weights pattern.The
vintage_weightsproperty follows the same pattern asperiod_weights: effect-specific → FlowSystem default → unitary fallback. This consistency is good.
481-495: Conditional vintage submodel creation is correct.The vintage
ShareAllocationModelis only created whenflow_system.vintages is not None, avoiding unnecessary model complexity for non-vintage scenarios.
513-531: Total constraint correctly excludes vintage.The comment explains the architectural decision: vintage has a different dimension than temporal/periodic, so the total is defined as
temporal + periodicfor backwards compatibility and per-period constraints. This is a reasonable design choice.
571-595: Overall constraints combine period and vintage contributions.The
_add_overall_constraintsmethod correctly computes the grand total asweighted_periods + weighted_vintages(when vintage exists). This provides users with a way to constrain the true overall effect.
897-922: Objective function correctly weights all effect categories.The objective combines:
- Temporal × period_weights × scenario_weights
- Periodic × period_weights × scenario_weights
- Vintage × vintage_weights × scenario_weights (if exists)
This ensures proper weighting across all dimensions.
862-874: Vintage target support in add_share_to_effects is well-guarded.The method correctly checks if
vintagesubmodel exists before adding shares, providing a clear error message if the FlowSystem lacks a vintage dimension.
940-953: Cross-effect vintage shares validation is consistent.The validation that both source and target effects have vintage submodels before adding shares prevents runtime errors from dimension mismatches.
| ## Basic Usage | ||
|
|
||
| ```python | ||
| solar_timing = fx.InvestmentParameters( | ||
| lifetime=10, # Investment lasts 10 periods | ||
| minimum_size=50, | ||
| maximum_size=200, | ||
| effects_of_size={'costs': 15000}, | ||
| effects_per_size={'costs': 1200}, | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
Basic usage example uses sizing parameter names instead of investment-specific ones
In the “Basic Usage” snippet you configure:
fx.InvestmentParameters(
lifetime=10,
minimum_size=50,
maximum_size=200,
effects_of_size={'costs': 15000},
effects_per_size={'costs': 1200},
)Per the InvestmentParameters API (and the later “Investment-Period Effects” examples), these fields should be effects_of_investment and effects_of_investment_per_size, not the sizing-style effects_of_size/effects_per_size. Otherwise readers will copy an example that does not match the class signature.
Suggestion:
- effects_of_size={'costs': 15000},
- effects_per_size={'costs': 1200},
+ effects_of_investment={'costs': 15000},
+ effects_of_investment_per_size={'costs': 1200},🤖 Prompt for AI Agents
In docs/user-guide/mathematical-notation/features/InvestmentParameters.md around
lines 29-39, the example uses sizing parameter names effects_of_size and
effects_per_size which do not match the InvestmentParameters API; update the
snippet to use effects_of_investment and effects_of_investment_per_size instead
(preserve values and context) so the example matches the class signature and
later examples.
flixopt/features.py
Outdated
| previous_level=0 if self.parameters.previous_lifetime == 0 else self.size.isel(period=0), | ||
| coord='period', | ||
| ) |
There was a problem hiding this comment.
Potential issue with previous_lifetime conditional.
The ternary 0 if self.parameters.previous_lifetime == 0 else self.size.isel(period=0) compares a DataArray to 0. After transform_data, previous_lifetime is a DataArray. This comparison may not work as expected.
After transform_data, previous_lifetime is a DataArray. The comparison self.parameters.previous_lifetime == 0 returns a DataArray of booleans, not a single boolean. Consider using .all() or .item():
- previous_level=0 if self.parameters.previous_lifetime == 0 else self.size.isel(period=0),
+ previous_level=0 if (self.parameters.previous_lifetime == 0).all() else self.size.isel(period=0),| def _track_lifetime(self): | ||
| """Create constraints that link investment period to decommissioning period based on lifetime.""" | ||
| periods = self._model.flow_system._fit_coords( | ||
| 'periods', self._model.flow_system.periods.values, dims=['period', 'scenario'] | ||
| ) | ||
|
|
||
| # Calculate decommissioning periods (vectorized) | ||
| import xarray as xr | ||
|
|
||
| is_first = periods == periods.isel(period=0) | ||
| decom_period = periods + self.parameters.lifetime - xr.where(is_first, self.parameters.previous_lifetime, 0) | ||
|
|
||
| # Map to available periods (drop invalid ones for sel to work) | ||
| valid = decom_period.where(decom_period <= self._model.flow_system.periods.values[-1], drop=True) | ||
| avail_decom = periods.sel(period=valid, method='bfill').assign_coords(period=valid.period) | ||
|
|
||
| # One constraint per unique decommissioning period | ||
| for decom_val in np.unique(avail_decom.values): | ||
| mask = (avail_decom == decom_val).reindex_like(periods).fillna(0) | ||
| self.add_constraints( | ||
| self.investment_occurs.where(mask).sum('period') == self.decommissioning_occurs.sel(period=decom_val), | ||
| short_name=f'size|lifetime{int(decom_val)}', | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for tests that exercise the lifetime tracking logic
rg -n "lifetime|_track_lifetime" tests/ --type=py -A 5 -B 2 | head -150Repository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Also check the full implementation context in features.py
wc -l flixopt/features.pyRepository: flixOpt/flixopt
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Search for any TODOs, FIXMEs, or issues related to _track_lifetime
rg -n "_track_lifetime|lifetime.*constraint" flixopt/features.py -B 2 -A 10Repository: flixOpt/flixopt
Length of output: 1287
🏁 Script executed:
#!/bin/bash
# Read the complete _track_lifetime method
sed -n '287,309p' flixopt/features.pyRepository: flixOpt/flixopt
Length of output: 1307
🏁 Script executed:
#!/bin/bash
# Search for any test files that might test investment features
fd -e py tests/ | grep -i "invest\|lifetime\|feature" | head -20Repository: flixOpt/flixopt
Length of output: 388
🏁 Script executed:
#!/bin/bash
# Look for integration tests that might indirectly test this
rg -l "investment\|decommission" tests/ --type=pyRepository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check if there are any validation or test scripts at the repository root
fd -e py . -maxdepth 2 | grep -i testRepository: flixOpt/flixopt
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Search for test directory structure
find . -type d -name "test*" 2>/dev/null | head -20Repository: flixOpt/flixopt
Length of output: 66
🏁 Script executed:
#!/bin/bash
# Look at the Investment class to understand parameters
rg -n "class.*Investment|self.parameters.lifetime|self.parameters.previous_lifetime" flixopt/features.py -B 2 -A 5 | head -60Repository: flixOpt/flixopt
Length of output: 1297
🏁 Script executed:
#!/bin/bash
# Check what decommissioning_occurs and investment_occurs are
rg -n "decommissioning_occurs|investment_occurs" flixopt/features.py -B 2 | head -80Repository: flixOpt/flixopt
Length of output: 2354
🏁 Script executed:
#!/antml
# Search for any examples or documentation about lifetime
rg -n "lifetime" docs/ README* --type=md 2>/dev/null | head -50Repository: flixOpt/flixopt
Length of output: 1533
🏁 Script executed:
#!/bin/bash
# List test files to find investment-related tests
ls -la tests/Repository: flixOpt/flixopt
Length of output: 2087
🏁 Script executed:
#!/bin/bash
# Look for any test that mentions InvestmentModel or lifetime
rg -l "InvestmentModel|lifetime" tests/ --type=py 2>/dev/nullRepository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
#!/antml
# Read the documentation about the lifetime constraint logic
cat docs/user-guide/mathematical-notation/features/InvestmentParameters.md | head -100Repository: flixOpt/flixopt
Length of output: 2369
🏁 Script executed:
#!/antml
# Look for issues with the sel() logic - check what valid contains
rg -n "\.sel\(period=" flixopt/features.py -B 3 -A 3Repository: flixOpt/flixopt
Length of output: 810
🏁 Script executed:
#!/antml
# Search for investment tests in test_component.py
rg -n "investment|Investment|lifetime|Lifetime" tests/test_component.py -A 5 -B 2 | head -150Repository: flixOpt/flixopt
Length of output: 2216
🏁 Script executed:
#!/antml
# Check test_functional.py which might have functional tests
rg -n "investment|Investment|lifetime|Lifetime" tests/test_functional.py -A 5 -B 2 | head -150Repository: flixOpt/flixopt
Length of output: 5156
🏁 Script executed:
#!/antml
# Look for any GitHub issues or comments about this specific logic
rg -n "method.*bfill|sel.*period.*valid" flixopt/ --type=py -B 3 -A 3Repository: flixOpt/flixopt
Length of output: 1217
Lifetime tracking logic lacks test coverage and contains unclear coordinate handling that should be verified.
While the vectorized calculation is sophisticated, there are no dedicated tests for the _track_lifetime() method or the decommissioning period constraint logic. The method='bfill' selector at line 301 maps calculated float decommissioning periods to available periods with backfilling, then reassigns coordinates—this behavior needs clarification through tests or documentation. The constraint at line 307 linking investment_occurs to decommissioning_occurs per unique decommissioning period requires validation that the mask reindexing and coordinate reassignment produce the intended lifetime relationship.
…ndling that needed clarification and test coverage.
Changes Made
1. Simplified _track_lifetime() logic (flixopt/features.py:287-336):
- Rewrote from vectorized xarray operations to explicit for-loop logic
- Added comprehensive docstring with examples
- Made the investment-to-decommissioning mapping explicit and readable
2. Fixed _SizeParameters isinstance checks:
- Updated elements.py to use _SizeParameters base class for all size parameter checks
- Updated components.py similarly
- This ensures InvestmentParameters is properly recognized alongside SizingParameters and deprecated InvestParameters
3. Added differentiated model selection:
- _create_investment_model() now selects:
- InvestmentModel for InvestmentParameters (with lifetime tracking)
- SizingModel for SizingParameters or deprecated InvestParameters
4. Fixed parameter name: Changed previous_level to initial_level in BoundingPatterns.link_changes_to_level_with_binaries() call
5. Added test coverage (tests/test_functional.py:736-831):
- New test_investment_parameters_with_lifetime test
- Tests multi-period optimization with InvestmentParameters
- Verifies investment_occurs and decommissioning_occurs variables exist
- Validates investment occurs at most once
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Release Notes
New Features
SizingParametersandInvestmentParametersclasses to public API for enhanced sizing and investment modeling.Documentation
InvestmentParametersandSizingParameterswith usage examples and mathematical constraints.Refactor
✏️ Tip: You can customize this high-level summary in your review settings.