Feature/rename calculation #482
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 1 minutes and 59 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 (15)
WalkthroughIntroduces a new optimization module and public classes (Optimization, ClusteredOptimization, SegmentedOptimization), renames Aggregation→Clustering and CalculationResults→Results, and preserves legacy names as deprecated wrappers. Removes many legacy **kwargs/deprecation shims across components, effects, converters, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DepWrapper as Deprecated Wrapper\n(Calculation*)
participant CanonicalOpt as Optimization family
participant FlowSystem
participant Solver
User->>DepWrapper: instantiate legacy class (e.g. FullCalculation)
Note right of DepWrapper#lightblue: emit deprecation warning
DepWrapper->>CanonicalOpt: delegate initialization & calls
DepWrapper->>FlowSystem: copy/prepare flow system
User->>DepWrapper: do_modeling() / solve()
DepWrapper->>CanonicalOpt: delegate do_modeling()/solve()
CanonicalOpt->>FlowSystem: build model
CanonicalOpt->>Solver: run solve
CanonicalOpt-->>User: store and return Results
sequenceDiagram
participant User
participant SegOpt as SegmentedOptimization
participant SubCalc as Optimization (segment)
participant Solver
participant FlowSystem
User->>SegOpt: __init__(flow_system, timesteps_per_segment, overlap, ...)
SegOpt->>SegOpt: _calculate_timesteps_per_segment()
SegOpt->>SegOpt: _create_sub_calculations()
loop per segment i
SegOpt->>SubCalc: do_modeling()
SegOpt->>SegOpt: _transfer_start_values(i)
SubCalc->>Solver: solve segment i
SegOpt->>SegOpt: collect durations & segment Results
end
SegOpt-->>User: return SegmentedResults
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
flixopt/results.py (2)
54-80: Results docstring and deprecation message still reference non‑existentOptimizationResultsThe
Results.__init__docstring and the deprecation warning insideResults.__init__both tell users to switch fromCalculationResultstoOptimizationResults, but the new public class name isResults, andCalculationResultsis now just a thin deprecated subclass.This is confusing for users and tooling and should be aligned with the actual API.
Suggested diff to align wording with the current classes:
@@ - def __init__( + def __init__( self, solution: xr.Dataset, @@ - ): - """Initialize CalculationResults with optimization data. - Usually, this class is instantiated by the Calculation class, or by loading from file. + ): + """Initialize Results with optimization data. + Usually, this class is instantiated by an Optimization object via `Results.from_optimization()` + or by loading from file using `Results.from_file()`. @@ - Deprecated: - flow_system: Use flow_system_data instead. - - Note: - CalculationResults is deprecated. Use OptimizationResults instead. + Deprecated: + flow_system: Use flow_system_data instead. + + Note: + The legacy alias `CalculationResults` is deprecated. Use `Results` instead. @@ - # Deprecation warning for CalculationResults - if self.__class__.__name__ == 'CalculationResults': - warnings.warn( - 'CalculationResults is deprecated and will be removed in a future version. ' - 'Use OptimizationResults instead.', - DeprecationWarning, - stacklevel=2, - ) + # Deprecation warning for the legacy CalculationResults alias + if self.__class__.__name__ == 'CalculationResults': + warnings.warn( + 'CalculationResults is deprecated and will be removed in a future version. ' + 'Use Results instead.', + DeprecationWarning, + stacklevel=2, + )Also applies to: 212-227, 229-235, 1108-1138
2000-2057: SegmentedResults / SegmentedCalculationResults deprecation path is inconsistent and may warn twiceTwo issues around
SegmentedResultsand the deprecatedSegmentedCalculationResultsalias:
Conflicting names in warnings
- In
SegmentedResults.__init__(Line 2150 and following), the deprecation message says “Use SegmentedOptimizationResults instead.” but there is no such class; the new public name isSegmentedResults.- In
SegmentedCalculationResults.__init__(Lines 2393-2401), the message correctly says “Use SegmentedResults instead.”This inconsistency will be confusing.
Potential double warning for SegmentedCalculationResults
Instantiating
SegmentedCalculationResultswill:
- Trigger the warning in
SegmentedCalculationResults.__init__.- Then call
super().__init__, whereSegmentedResults.__init__checksself.__class__.__name__ == 'SegmentedCalculationResults'again and emits a second warning.Type hint still tied to
CalculationResults
SegmentedResults.__init__currently takessegment_results: list[CalculationResults], butSegmentedResults.from_optimization()actually passes a list ofResultsinstances ([calc.results for calc in optimization.sub_calculations]). Typing works at runtime, but the hint is stale.Suggested diff to simplify the deprecation path and modernize the type hint:
@@ - def __init__( - self, - segment_results: list[CalculationResults], - all_timesteps: pd.DatetimeIndex, - timesteps_per_segment: int, - overlap_timesteps: int, - name: str, - folder: pathlib.Path | None = None, - ): - # Deprecation warning for SegmentedCalculationResults - if self.__class__.__name__ == 'SegmentedCalculationResults': - warnings.warn( - 'SegmentedCalculationResults is deprecated and will be removed in a future version. ' - 'Use SegmentedOptimizationResults instead.', - DeprecationWarning, - stacklevel=2, - ) - - self.segment_results = segment_results + def __init__( + self, + segment_results: list[Results], + all_timesteps: pd.DatetimeIndex, + timesteps_per_segment: int, + overlap_timesteps: int, + name: str, + folder: pathlib.Path | None = None, + ): + self.segment_results = segment_results @@ class SegmentedCalculationResults(SegmentedResults): - def __init__(self, *args, **kwargs): - # Only warn if directly instantiating SegmentedCalculationResults (not subclasses) - if self.__class__.__name__ == 'SegmentedCalculationResults': - warnings.warn( - 'SegmentedCalculationResults is deprecated and will be removed in a future version. ' - 'Use SegmentedResults instead.', - DeprecationWarning, - stacklevel=2, - ) - super().__init__(*args, **kwargs) + def __init__(self, *args, **kwargs): + # Only warn if directly instantiating SegmentedCalculationResults (not subclasses) + if self.__class__.__name__ == 'SegmentedCalculationResults': + warnings.warn( + 'SegmentedCalculationResults is deprecated and will be removed in a future version. ' + 'Use SegmentedResults instead.', + DeprecationWarning, + stacklevel=2, + ) + super().__init__(*args, **kwargs)This way:
- Users see a single, consistent warning when using the deprecated alias.
- The constructor type hint matches what
from_optimization()actually passes.Also applies to: 2142-2158, 2386-2417
🧹 Nitpick comments (2)
flixopt/optimization.py (2)
25-31: Redundant InvestmentModel import in_Optimization.main_results
InvestmentModelis imported at the module level (Line 30) and again insidemain_resultsviafrom flixopt.features import InvestmentModel(Line 99). The inner import is redundant and slightly obscures where the dependency comes from.You can safely rely on the existing top‑level import and drop the inner one.
- @property - def main_results(self) -> dict[str, int | float | dict]: - from flixopt.features import InvestmentModel - + @property + def main_results(self) -> dict[str, int | float | dict]:Also applies to: 97-142
236-278: Optimization / SegmentedOptimization workflow and BC behavior look correct; only messages still mention “Calculation”The core workflow changes look good:
Optimization.solve()auto‑callsdo_modeling()when needed, uses_Solver.options, logs main results, and populatesself.results = Results.from_optimization(self).SegmentedOptimization.do_modeling_and_solve()builds per‑segmentOptimizationinstances, solves them with optional progress bar suppression, aggregatesdurations, and setsself.results = SegmentedResults.from_optimization(self).This preserves the old behavior while routing everything through the new
Results/SegmentedResultscontainers.The only minor nit is some log/exception texts still using “Calculation”, e.g.:
- Line 257:
CalculationResultsPaths- Line 607:
'Investments are not supported in Segmented Calculation!'
CalculationResultsPathsis a deliberate BC name, but the human‑readable message in_solve_single_segment()could be updated to match the new terminology (e.g. “SegmentedOptimization”) when you next touch this area.Also applies to: 423-689
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
flixopt/__init__.py(3 hunks)flixopt/calculation.py(5 hunks)flixopt/optimization.py(1 hunks)flixopt/results.py(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
flixopt/calculation.py (2)
flixopt/optimization.py (5)
ClusteredOptimization(280-420)Optimization(170-277)_Optimization(43-167)SegmentedOptimization(423-746)active_timesteps(157-163)flixopt/aggregation.py (1)
AggregationParameters(239-292)
flixopt/optimization.py (8)
flixopt/aggregation.py (7)
Aggregation(41-236)AggregationModel(295-389)AggregationParameters(239-292)labels_for_high_peaks(283-284)labels_for_low_peaks(287-288)cluster(83-107)plot(144-178)flixopt/config.py (3)
CONFIG(58-407)Solving(155-177)Plotting(179-213)flixopt/core.py (6)
DataConverter(167-574)TimeSeriesData(36-164)drop_constant_arrays(604-634)to_dataarray(410-524)is_timeseries_data(132-134)from_dataarray(117-129)flixopt/features.py (1)
InvestmentModel(24-144)flixopt/results.py (11)
flow_system(352-369)Results(54-1105)SegmentedResults(2000-2383)objective(321-328)size(1989-1997)constraints(338-342)constraints(1162-1170)variables(331-335)variables(1151-1159)from_optimization(184-200)from_optimization(2097-2113)flixopt/flow_system.py (6)
FlowSystem(38-1616)sel(1320-1348)connect_and_transform(724-737)coords(1117-1123)_connect_network(984-1019)flows(1085-1091)flixopt/solvers.py (2)
_Solver(16-41)options(34-36)flixopt/io.py (5)
round_nested_floats(42-86)CalculationResultsPaths(599-650)document_linopy_model(473-524)format_yaml_string(254-301)suppress_output(999-1051)
flixopt/__init__.py (1)
flixopt/optimization.py (3)
ClusteredOptimization(280-420)Optimization(170-277)SegmentedOptimization(423-746)
flixopt/results.py (4)
flixopt/structure.py (6)
CompositeContainerMixin(1181-1357)flow_system(327-345)to_dataset(775-798)to_netcdf(800-816)variables(1497-1503)constraints(1488-1494)flixopt/flow_system.py (2)
to_dataset(539-551)to_netcdf(612-626)flixopt/optimization.py (2)
summary(144-154)all_timesteps(745-746)flixopt/io.py (2)
document_linopy_model(473-524)load_json(94-110)
🔇 Additional comments (3)
flixopt/__init__.py (1)
18-19: Public API surface cleanly exposes new and deprecated classesThe module now re‑exports the new
Optimization,ClusteredOptimization, andSegmentedOptimizationclasses while still exposing legacyFullCalculation,AggregatedCalculation, andSegmentedCalculationunder a clearly marked deprecated section in__all__. This is a solid BC strategy and keeps the transition path obvious for users.Also applies to: 35-37, 52-59
flixopt/calculation.py (2)
41-48: Deprecation helper and Calculation/FullCalculation wrappers look sound
_deprecation_warningcentralizes messaging and thestacklevel=3will correctly point at user call sites when these classes are instantiated viaCalculation/FullCalculation. Both wrappers preserve the original constructor signature and delegate cleanly to_Optimization, so runtime behavior should match the newOptimizationclass aside from the warning.Also applies to: 50-77
79-109: AggregatedCalculation / SegmentedCalculation wrappers preserve behavior with clear deprecation path
AggregatedCalculationandSegmentedCalculationmirror the parameter lists ofClusteredOptimizationandSegmentedOptimizationand simply forward to the new implementations after emitting a deprecation warning. The super calls are correctly ordered, so aggregation/segmentation behavior is preserved while steering users toward the new names via both docstrings and runtime warnings.Also applies to: 111-144, 146-172, 174-174
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 (1)
CHANGELOG.md(1 hunks)
🔇 Additional comments (1)
CHANGELOG.md (1)
62-70: Address missing test verification before merge.According to the PR objectives, both "I have tested my changes" and "Existing tests still pass" are unchecked. Given that this is a major refactoring affecting the public API with widespread class renames, merging without verification that existing tests pass introduces significant risk of breaking changes going undetected.
Please confirm that:
- All existing tests pass with these class renames
- Backward compatibility with deprecated wrappers has been tested
- Public API exports in
__init__.pyare correctly updated
CHANGELOG.md
Outdated
| **Class renaming:** | ||
| - `Calculation` → `Optimization` | ||
| - `AggregatedCalculation` → `AggregatedOptimization` | ||
| - `SegmentedCalculation` → `SegmentedOptimization` | ||
| - `CalculationResults` → `Results` | ||
| - `SegmentedCalculationResults` → `SegmentedResults` | ||
| - Module: `calculation.py` → `optimization.py` | ||
| Old names remain available with deprecation warnings (removed in v5.0.0). |
There was a problem hiding this comment.
🧩 Analysis chain
Verify class rename accuracy against implementation.
The changelog lists AggregatedOptimization as the new name, but the AI-generated summary references ClusteredOptimization (described as "with aggregation"). Additionally, the summary mentions FullCalculation as a backward-compatible wrapper, but this class isn't explicitly documented in the changelog's rename list.
Before merging, please confirm:
- Is the correct new name
AggregatedOptimizationorClusteredOptimization? - Is
FullCalculationbeing renamed or remaining as a deprecated wrapper alongside the new names? - Are there any other classes in the
Calculationfamily not documented here?
To verify, you can inspect the actual implementation:
🏁 Script executed:
#!/bin/bash
# Verify the actual class names in the new optimization module
echo "=== Classes in flixopt/optimization.py ==="
rg -nP '^\s*class\s+\w+' flixopt/optimization.py
echo "=== Deprecated wrappers in flixopt/calculation.py ==="
rg -nP '^\s*class\s+\w+' flixopt/calculation.py
echo "=== Results classes in flixopt/results.py ==="
rg -nP '^\s*class\s+(CalculationResults|Results|SegmentedCalculationResults|SegmentedResults)' flixopt/results.pyLength of output: 792
Correct the changelog: AggregatedCalculation renames to ClusteredOptimization, not AggregatedOptimization. Also, FullCalculation is missing from the rename list.
The implementation verification shows two issues in lines 62-70:
-
Line 65 contains incorrect class name: The changelog states
AggregatedCalculation→AggregatedOptimization, but the actual implementation (optimization.py line 280) shows it renames toClusteredOptimization. The oldAggregatedCalculationinherits from_ClusteredOptimization, confirming the correct target name. -
Line 62-70 omits FullCalculation: The class
FullCalculation(calculation.py line 79) exists in the codebase but is not listed in the changelog's rename documentation. While it remains unchanged as a deprecated wrapper, it should be explicitly documented to provide complete migration guidance.
Update the changelog to:
- Change line 65 from
AggregatedCalculation→AggregatedOptimizationtoAggregatedCalculation→ClusteredOptimization - Add
FullCalculationto the list with clarification that it remains available as a deprecated wrapper
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 62 to 70, correct the rename mapping and add the
missing class: change the line that currently documents "AggregatedCalculation →
AggregatedOptimization" to "AggregatedCalculation → ClusteredOptimization" to
match the implementation, and insert an entry for "FullCalculation" indicating
it remains available as a deprecated wrapper (not renamed) so the list documents
this class for migration completeness.
…s to use "clustering" terminology for better consistency with the ClusteredOptimization class
name.
Changes Made:
1. Module renamed:
- aggregation.py → clustering.py (via git mv)
2. Classes renamed:
- Aggregation → Clustering
- AggregationParameters → ClusteringParameters
- AggregationModel → ClusteringModel
3. Updated references in:
- optimization.py: All imports, attributes, and method parameters
- calculation.py: Import statement (for backward compat type hints)
- __init__.py: Exports (includes both old and new names)
- Module docstrings updated
4. Attribute updates:
- ClusteredOptimization.clustering_parameters
- ClusteredOptimization.clustering
- ClusteredOptimization.clustering_model
- self.durations['clustering']
- Label strings: 'Clustering' instead of 'Aggregation'
5. Backward compatibility:
- Added deprecated aliases in clustering.py for all old class names
- Old names exported from __init__.py with deprecation warnings
- All warnings point to v6.0.0 removal
6. Changelog updated: Added all clustering renames to the breaking changes section
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
flixopt/clustering.py (1)
320-351: Penalty expression is non‑scalar and will breakadd_share_to_penalty.In
ClusteringModel.do_modeling, this block:penalty = self.clustering_parameters.penalty_of_period_freedom if (self.clustering_parameters.percentage_of_period_freedom > 0) and penalty != 0: for variable in self.variables_direct.values(): self._model.effects.add_share_to_penalty('Clustering', variable * penalty)passes a time‑indexed variable to
add_share_to_penalty. Sinceadd_share_to_penaltyrequires a scalar expression (expression.ndim == 0),variable * penaltywill still have atimedimension and raise aTypeErrorwhenever period freedom and a non‑zero penalty are used.You likely want to penalize the total number of corrections per variable. That requires reducing over dimensions before adding the share, e.g.:
- for variable in self.variables_direct.values(): - self._model.effects.add_share_to_penalty('Clustering', variable * penalty) + for variable in self.variables_direct.values(): + # Aggregate correction indicators to a scalar before adding as penalty share + penalty_expr = variable.sum() + self._model.effects.add_share_to_penalty('Clustering', penalty_expr * penalty)(If
.sum()still leaves dimensions, apply an additional reduction so the final expression passed in is truly scalar.)Also applies to: 386-392
flixopt/results.py (1)
28-28: Update TYPE_CHECKING imports to reference the new Optimization classes.The TYPE_CHECKING block imports the deprecated
CalculationandSegmentedCalculationnames from.calculation, but these are not actually used in any type hints throughoutresults.py. The actual type annotations use the newOptimizationandSegmentedOptimizationclasses.Update line 28 to import the new names instead, or remove the imports entirely if they're unused.
🧹 Nitpick comments (2)
flixopt/clustering.py (1)
320-327: Optional: makecomponents_to_clusterizeexplicit and avoid truthiness checks.Using
if not self.components_to_clusterize:means an empty list will cluster all components, which is surprising if someone builds the list programmatically.Consider treating
Noneas “all components” and a list (even if empty) as “exactly these components”, e.g.:- if not self.components_to_clusterize: - components = self.flow_system.components.values() - else: - components = [component for component in self.components_to_clusterize] + if self.components_to_clusterize is None: + components = self.flow_system.components.values() + else: + components = list(self.components_to_clusterize)This keeps behavior clear and avoids relying on list truthiness.
flixopt/results.py (1)
213-229: Consider clarifying instantiation patterns in docstring.The docstring mentions that "this class is instantiated by an Optimization object via
Results.from_optimization()", which is helpful. However, it could be slightly clearer that direct instantiation via__init__is primarily for internal use or when loading from files.Consider this enhancement:
def __init__( self, solution: xr.Dataset, flow_system_data: xr.Dataset, name: str, summary: dict, folder: pathlib.Path | None = None, model: linopy.Model | None = None, **kwargs, # To accept old "flow_system" parameter ): """Initialize Results with optimization data. - Usually, this class is instantiated by an Optimization object via `Results.from_optimization()` - or by loading from file using `Results.from_file()`. + + Note: + Typically, you should use the factory methods `Results.from_optimization()` + or `Results.from_file()` instead of calling this constructor directly. Args:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)flixopt/__init__.py(3 hunks)flixopt/calculation.py(5 hunks)flixopt/clustering.py(9 hunks)flixopt/optimization.py(1 hunks)flixopt/results.py(17 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
flixopt/optimization.py (8)
flixopt/clustering.py (8)
Clustering(44-239)ClusteringModel(298-392)ClusteringParameters(242-295)do_modeling(320-350)labels_for_high_peaks(286-287)labels_for_low_peaks(290-291)cluster(86-110)plot(147-181)flixopt/core.py (6)
DataConverter(167-574)TimeSeriesData(36-164)drop_constant_arrays(604-634)to_dataarray(410-524)is_timeseries_data(132-134)from_dataarray(117-129)flixopt/features.py (1)
InvestmentModel(24-144)flixopt/results.py (11)
flow_system(353-370)Results(55-1106)SegmentedResults(2001-2375)objective(322-329)size(1990-1998)constraints(339-343)constraints(1163-1171)variables(332-336)variables(1152-1160)from_optimization(185-201)from_optimization(2098-2114)flixopt/flow_system.py (6)
FlowSystem(38-1616)sel(1320-1348)connect_and_transform(724-737)coords(1117-1123)_connect_network(984-1019)flows(1085-1091)flixopt/solvers.py (2)
_Solver(16-41)options(34-36)flixopt/structure.py (3)
FlowSystemModel(81-274)copy(934-946)solution(156-179)flixopt/io.py (4)
round_nested_floats(42-86)CalculationResultsPaths(599-650)document_linopy_model(473-524)suppress_output(999-1051)
flixopt/calculation.py (2)
flixopt/optimization.py (5)
ClusteredOptimization(278-417)Optimization(168-275)_Optimization(43-165)SegmentedOptimization(420-743)active_timesteps(155-161)flixopt/clustering.py (2)
AggregationParameters(415-420)ClusteringParameters(242-295)
flixopt/results.py (4)
flixopt/structure.py (6)
CompositeContainerMixin(1181-1357)flow_system(327-345)to_dataset(775-798)to_netcdf(800-816)variables(1497-1503)constraints(1488-1494)flixopt/flow_system.py (2)
to_dataset(539-551)to_netcdf(612-626)flixopt/optimization.py (2)
summary(142-152)all_timesteps(742-743)flixopt/io.py (2)
document_linopy_model(473-524)load_json(94-110)
flixopt/__init__.py (3)
flixopt/calculation.py (3)
AggregatedCalculation(112-144)FullCalculation(80-109)SegmentedCalculation(147-172)flixopt/clustering.py (2)
AggregationParameters(415-420)ClusteringParameters(242-295)flixopt/optimization.py (3)
ClusteredOptimization(278-417)Optimization(168-275)SegmentedOptimization(420-743)
flixopt/clustering.py (2)
flixopt/structure.py (4)
Submodel(1360-1539)FlowSystemModel(81-274)variables_direct(1478-1480)add_constraints(1395-1404)flixopt/effects.py (1)
add_share_to_penalty(776-779)
⏰ 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.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
🔇 Additional comments (2)
flixopt/calculation.py (1)
2-12: Deprecation wrappers for Calculation look consistent and low‑risk.*The module‑level docs,
_deprecation_warninghelper, and the four wrapper classes correctly route to the new optimization bases while emitting clear deprecation warnings. Signatures and super‑calls match the underlyingOptimization,ClusteredOptimization, andSegmentedOptimizationconstructors, so existing imports fromflixopt.calculationshould continue to work as intended.Also applies to: 19-28, 42-49, 51-175
flixopt/optimization.py (1)
1-9: Optimization, ClusteredOptimization, and SegmentedOptimization are wired coherently to the existing modeling stack.The new optimization module cleanly centralizes common behavior in
_Optimizationand exposes the three strategies with consistent modeling/solving flows, deprecation handling foractive_timesteps, and sensible logging/error handling. The transition from the old Calculation classes to these Optimization types appears mechanically sound and aligns with the clustering and results infrastructure already in the codebase.Also applies to: 43-276, 278-417, 420-743
| **Class and module renaming:** | ||
| - `FullCalculation` → `Optimization` | ||
| - `AggregatedCalculation` → `ClusteredOptimization` | ||
| - `SegmentedCalculation` → `SegmentedOptimization` | ||
| - `CalculationResults` → `Results` | ||
| - `SegmentedCalculationResults` → `SegmentedResults` | ||
| - `Aggregation` → `Clustering` | ||
| - `AggregationParameters` → `ClusteringParameters` | ||
| - `AggregationModel` → `ClusteringModel` | ||
| - Module: `calculation.py` → `optimization.py` | ||
| - Module: `aggregation.py` → `clustering.py` | ||
| Old names remain available with deprecation warnings (removed in v5.0.0). | ||
There was a problem hiding this comment.
Rename list matches implementation; consider also documenting Calculation → Optimization.
The class/module rename bullets now line up with the new API (AggregatedCalculation → ClusteredOptimization, FullCalculation → Optimization, results and aggregation/clustering mappings) and the deprecation behavior in code.
For completeness of the migration notes, consider adding an entry for the plain Calculation → Optimization rename as well, since flixopt/calculation.py still exposes Calculation as a deprecated wrapper.
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 62 to 75, the rename list omits the plain
Calculation → Optimization mapping; add a bullet documenting that Calculation
was renamed to Optimization (and note that the old Calculation remains available
as a deprecated wrapper in flixopt/calculation.py and will be removed in v5.0.0)
so the migration notes fully cover all deprecated aliases.
| # Import old Calculation classes for backwards compatibility (deprecated) | ||
| from .calculation import AggregatedCalculation, FullCalculation, SegmentedCalculation | ||
| from .clustering import AggregationParameters, ClusteringParameters # AggregationParameters is deprecated | ||
| from .components import ( |
There was a problem hiding this comment.
Calculation is not re‑exported at package root, breaking top‑level backward compatibility.
This module promises that legacy Calculation* names remain available as deprecated wrappers, but __init__.py only imports/exports FullCalculation, AggregatedCalculation, and SegmentedCalculation. Code that previously did:
from flixopt import Calculation
# or
import flixopt as fx; fx.Calculation(...)will now fail, even though flixopt.calculation.Calculation still exists.
To keep the public API backward compatible, consider:
-from .calculation import AggregatedCalculation, FullCalculation, SegmentedCalculation
+from .calculation import AggregatedCalculation, Calculation, FullCalculation, SegmentedCalculation
...
- # Old Calculation classes (deprecated, for backwards compatibility)
- 'FullCalculation',
- 'AggregatedCalculation',
- 'SegmentedCalculation',
+ # Old Calculation classes (deprecated, for backwards compatibility)
+ 'Calculation',
+ 'FullCalculation',
+ 'AggregatedCalculation',
+ 'SegmentedCalculation',This aligns the package surface with the deprecation story in flixopt/calculation.py and the changelog.
Also applies to: 52-60, 66-67
🤖 Prompt for AI Agents
In flixopt/__init__.py around lines 17 to 20, the legacy alias Calculation is
not re-exported which breaks top-level backward compatibility; import
Calculation from .calculation and add it to the module exports (and __all__ if
present) alongside FullCalculation, AggregatedCalculation, and
SegmentedCalculation so from flixopt import Calculation and attribute access
like flixopt.Calculation continue to work as before.
# Conflicts: # flixopt/core.py # flixopt/interface.py # test_deprecations.py
…calculation # Conflicts: # flixopt/calculation.py
…calculation # Conflicts: # flixopt/calculation.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
flixopt/__init__.py (1)
18-20: MissingCalculationexport breaks backwards compatibility.The
Calculationclass exists inflixopt/calculation.pybut is not imported or exported at the package level. Users who previously usedfrom flixopt import Calculationorfx.Calculation(...)will encounterImportErrororAttributeError.Apply this diff to restore backwards compatibility:
# Import old Calculation classes for backwards compatibility (deprecated) -from .calculation import AggregatedCalculation, FullCalculation, SegmentedCalculation +from .calculation import AggregatedCalculation, Calculation, FullCalculation, SegmentedCalculation from .clustering import AggregationParameters, ClusteringParameters # AggregationParameters is deprecatedAnd update
__all__:'SegmentedOptimization', # Old Calculation classes (deprecated, for backwards compatibility) + 'Calculation', 'FullCalculation', 'AggregatedCalculation',Also applies to: 57-60
flixopt/results.py (3)
154-163: Update docstring to reference the new class name.The docstring on line 155 still mentions "CalculationResults" but should reference "Results" to match the renamed class.
Apply this diff:
@classmethod def from_file(cls, folder: str | pathlib.Path, name: str) -> Results: - """Load CalculationResults from saved files. + """Load Results from saved files. Args: folder: Directory containing saved files.
232-238: Remove duplicate deprecation warning check.This deprecation warning check in
Results.__init__causes duplicate warnings when users instantiateCalculationResults:
CalculationResults.__init__(lines 1122-1127) warns whenself.__class__.__name__ == 'CalculationResults'- It calls
super().__init__()which triggers this warning again (lines 232-238)- Result: Users see the same deprecation warning twice
The correct pattern (as used in
SegmentedResults) is to only warn in the deprecated wrapper class, not in the base class.Apply this diff to remove the redundant check:
- # Deprecation warning for the legacy CalculationResults alias - if self.__class__.__name__ == 'CalculationResults': - warnings.warn( - f'CalculationResults is deprecated and will be removed in v{DEPRECATION_REMOVAL_VERSION}. Use Results instead.', - DeprecationWarning, - stacklevel=2, - ) - # Handle potential old "flow_system" parameter for backward compatibility
2121-2130: Update docstring to reference the new class name.The docstring on line 2122 still mentions "SegmentedCalculationResults" but should reference "SegmentedResults" to match the renamed class.
Apply this diff:
@classmethod def from_file(cls, folder: str | pathlib.Path, name: str) -> SegmentedResults: - """Load SegmentedCalculationResults from saved files. + """Load SegmentedResults from saved files. Args: folder: Directory containing saved files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.md(1 hunks)flixopt/__init__.py(3 hunks)flixopt/calculation.py(5 hunks)flixopt/clustering.py(10 hunks)flixopt/components.py(0 hunks)flixopt/core.py(0 hunks)flixopt/effects.py(0 hunks)flixopt/elements.py(0 hunks)flixopt/interface.py(0 hunks)flixopt/linear_converters.py(0 hunks)flixopt/optimization.py(1 hunks)flixopt/results.py(17 hunks)test_deprecations.py(0 hunks)tests/test_flow_system_resample.py(1 hunks)tests/test_invest_parameters_deprecation.py(0 hunks)
💤 Files with no reviewable changes (8)
- flixopt/core.py
- tests/test_invest_parameters_deprecation.py
- flixopt/interface.py
- test_deprecations.py
- flixopt/elements.py
- flixopt/components.py
- flixopt/effects.py
- flixopt/linear_converters.py
🧰 Additional context used
🧬 Code graph analysis (5)
flixopt/__init__.py (3)
flixopt/calculation.py (3)
AggregatedCalculation(115-147)FullCalculation(83-112)SegmentedCalculation(150-175)flixopt/clustering.py (2)
AggregationParameters(418-423)ClusteringParameters(245-298)flixopt/optimization.py (3)
ClusteredOptimization(278-418)Optimization(170-275)SegmentedOptimization(421-744)
flixopt/clustering.py (2)
flixopt/structure.py (2)
variables_direct(1480-1482)add_constraints(1397-1406)flixopt/effects.py (1)
add_share_to_penalty(599-602)
flixopt/calculation.py (2)
flixopt/optimization.py (5)
ClusteredOptimization(278-418)Optimization(170-275)_Optimization(45-167)SegmentedOptimization(421-744)active_timesteps(157-163)flixopt/clustering.py (2)
AggregationParameters(418-423)ClusteringParameters(245-298)
flixopt/results.py (4)
flixopt/structure.py (6)
CompositeContainerMixin(1183-1359)flow_system(329-347)to_dataset(777-800)to_netcdf(802-818)variables(1499-1505)constraints(1490-1496)flixopt/flow_system.py (2)
to_dataset(541-553)to_netcdf(614-628)flixopt/optimization.py (2)
summary(144-154)all_timesteps(743-744)flixopt/io.py (2)
document_linopy_model(475-526)load_json(96-112)
flixopt/optimization.py (6)
flixopt/clustering.py (8)
Clustering(46-242)ClusteringModel(301-395)ClusteringParameters(245-298)do_modeling(323-353)labels_for_high_peaks(289-290)labels_for_low_peaks(293-294)cluster(88-113)plot(150-184)flixopt/core.py (6)
DataConverter(127-534)TimeSeriesData(36-124)drop_constant_arrays(564-594)to_dataarray(370-484)is_timeseries_data(112-114)from_dataarray(97-109)flixopt/features.py (1)
InvestmentModel(24-144)flixopt/results.py (10)
flow_system(355-374)Results(57-1110)SegmentedResults(2005-2379)size(1994-2002)constraints(341-345)constraints(1167-1175)variables(334-338)variables(1156-1164)from_optimization(187-203)from_optimization(2102-2118)flixopt/flow_system.py (6)
FlowSystem(40-1618)sel(1322-1350)connect_and_transform(726-739)coords(1119-1125)_connect_network(986-1021)flows(1087-1093)flixopt/io.py (1)
CalculationResultsPaths(601-652)
🪛 GitHub Actions: Python Package CI/CD
flixopt/calculation.py
[error] 33-33: F401 'typing.Any' imported but unused.
🪛 GitHub Check: lint
flixopt/calculation.py
[failure] 40-40: Ruff (F401)
flixopt/calculation.py:40:26: F401 .solvers._Solver imported but unused
[failure] 37-37: Ruff (F401)
flixopt/calculation.py:37:52: F401 .clustering.ClusteringParameters imported but unused
[failure] 33-33: Ruff (F401)
flixopt/calculation.py:33:35: F401 typing.Any imported but unused
🔇 Additional comments (11)
flixopt/clustering.py (3)
1-17: LGTM!The module docstring and imports have been updated correctly to reflect the Clustering terminology. The addition of deprecation warning infrastructure is well-implemented.
46-395: LGTM!The class renamings (Aggregation → Clustering, AggregationParameters → ClusteringParameters, AggregationModel → ClusteringModel) are implemented consistently. All internal references, attributes, and model labels have been updated to use the new clustering terminology.
398-431: LGTM!The deprecation wrappers are well-implemented. Each deprecated class inherits from its renamed counterpart and emits a clear deprecation warning. The
stacklevel=3is appropriate for the call chain (user code → wrapper__init__→_create_deprecation_warning).CHANGELOG.md (1)
62-75: LGTM!The Breaking Changes section clearly documents all class and module renamings with a clear deprecation policy. The documentation aligns with the implementation across the codebase.
flixopt/calculation.py (2)
45-176: LGTM!The deprecation wrappers are well-implemented. Each deprecated class properly inherits from its renamed counterpart, emits a clear deprecation warning, and delegates to the parent constructor. The pattern is consistent across all wrapper classes.
178-178: LGTM!The
__all__export list correctly includes all deprecated wrapper classes for backwards compatibility.flixopt/optimization.py (4)
1-43: LGTM!The module docstring and imports are well-organized. The docstring correctly describes the three optimization types (Optimization, ClusteredOptimization, SegmentedOptimization) with clear explanations of their purposes.
45-168: LGTM!The
_Optimizationbase class provides a solid foundation for all optimization types. Key features:
- Proper FlowSystem validation and duplication to prevent sharing across calculations
- Clear deprecation handling for
active_timestepsparameter- Comprehensive
main_resultsproperty for solution summary- Good error handling (e.g., folder validation)
170-418: LGTM!Both
OptimizationandClusteredOptimizationclasses are well-implemented:Optimization highlights:
- Auto-modeling when calling
solve()improves user experience- Infeasibility handling saves model state for debugging
fix_sizes()method enables staged optimization workflowsClusteredOptimization highlights:
- Proper validation of time step consistency for clustering
- Clear error messages for unsupported features (scenarios)
- Comprehensive aggregation weight calculation from dataset attributes
421-744: LGTM!The
SegmentedOptimizationclass is well-designed for handling large-scale problems:Strengths:
- Comprehensive docstring with usage examples and design considerations
- Proper investment parameter detection and warning (critical limitation)
- Flexible output modes (progress bar vs. detailed logging)
- Robust value transfer mechanism between segments for continuity
- Good balance between segment overlap and computational efficiency
Implementation quality:
- Clean separation of concerns across helper methods
- Proper error handling and validation
- Duration tracking and aggregation across segments
flixopt/results.py (1)
57-149: Well-structured refactoring with proper backwards compatibility.The renaming from
CalculationResults→ResultsandSegmentedCalculationResults→SegmentedResultsis cleanly implemented:
- Deprecated wrapper classes properly inherit from the new base classes
- Factory methods correctly renamed:
from_calculation()→from_optimization()- Legacy methods in wrappers appropriately redirect to new equivalents
- Type hints consistently updated across
_ElementResults,_NodeResults,FlowResults, andSegmentedResults- Documentation and examples updated to reference new class names
The deprecation strategy provides a smooth migration path for users while clearly signaling the API changes.
Also applies to: 187-203, 1113-1144, 2005-2118, 2382-2414
|
Continued in #484 |
Description
Rename Calculation -> Optimization
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Breaking Changes
New Features
Deprecations
✏️ Tip: You can customize this high-level summary in your review settings.