Fix docs and add migration guide #404
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a v3 migration guide and docs/nav updates, removes some FAQ content, adjusts changelog generation, introduces several explicit type hints, changes AggregatedCalculation to separate clustering ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant AggregatedCalc as AggregatedCalculation
participant AggregationClust as Aggregation (clustering)
participant AggModel as AggregationModel
User->>AggregatedCalc: do_modeling(..., request aggregation)
AggregatedCalc->>AggregationClust: _perform_aggregation() → produce clusters
AggregationClust-->>AggregatedCalc: clustered data (stored in self.aggregation)
AggregatedCalc->>AggModel: instantiate AggregationModel(self.aggregation, ...)
AggregatedCalc-->>AggModel: store as self.aggregation_model
AggregatedCalc->>AggModel: aggregation_model.do_modeling()
AggModel-->>AggregatedCalc: aggregated submodel results
AggregatedCalc-->>User: combined results (including aggregation info)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mkdocs.yml (1)
112-121: Add missing support files for new MkDocs plugins
- SUMMARY.md is missing at the repo root; it’s required by
literate-nav(or updatenav_fileto the correct path).- scripts/gen_ref_pages.py is missing; it’s required by
gen-files.- Ensure
literate-navis installed in your docs environment (e.g., added to requirements).
🧹 Nitpick comments (8)
docs/user-guide/migration-guide-v3.md (4)
118-124: Missing import for numpy in example
np.array(...)is used withoutimport numpy as np. Add the import for a runnable snippet.storage = fx.Storage( 'storage', relative_minimum_charge_state=np.array([0.2, 0.2, 0.2, 0.2, 0.2]) # 5 values for 4 timesteps ) + # Add at top of snippet: + import numpy as np
348-366: Missing import for xarray in scenarios example
xr.DataArrayis used withoutimport xarray as xr. Add the import for clarity and runnable docs.# Define scenarios with probabilities scenarios = pd.Index(['low_demand', 'base', 'high_demand'], name='scenario') scenario_weights = [0.2, 0.6, 0.2] # Probabilities +import xarray as xr
257-266: Confirm list vs scalar for inputs/outputsElsewhere you use
outputs=[fx.Flow(...)]. Hereoutputs=flowandinputs=floware scalars. If the API expects a sequence, update to lists for consistency:
fx.Source('my_source', outputs=[flow])fx.Sink('my_sink', inputs=[flow])
540-549: Address markdownlint MD007 (unordered list indentation) warningsThe checklist and other nested lists within admonitions/tabs may violate MD007 (2-space indentation). Normalize list indentation (2 spaces per nesting level) and ensure list items align under their parents. Run markdownlint locally to auto-fix.
Based on static analysis hints
mkdocs.yml (4)
85-99: Remove redundantcodehilitewhen usingpymdownx.highlightMaterial recommends
pymdownx.highlight;codehiliteis redundant and can cause conflicts.markdown_extensions: - admonition - - codehilite - markdown_include.include: base_path: docs - pymdownx.highlight: anchor_linenums: true
94-111: Deduplicatepymdownx.snippetsentriesDefined twice (one bare, one configured). Keep a single configured entry.
- pymdownx.inlinehilite - - pymdownx.snippets - pymdownx.superfences - attr_list - abbr - md_in_html - footnotes - tables - - pymdownx.tabbed: + - pymdownx.tabbed: alternate_style: true - pymdownx.arithmatex: generic: true - pymdownx.emoji: emoji_index: !!python/name:material.extensions.emoji.twemoji emoji_generator: !!python/name:material.extensions.emoji.to_svg - pymdownx.snippets: base_path: ..
77-83: Theme features: duplicatecontent.code.copyRemove the duplicate entry.
- content.code.annotate - - content.tooltips - - content.code.copy + - content.tooltips
123-151: Mkdocstrings option comments don’t match values
inherited_members: false– comment says “Include members...”. Either set to true or fix the comment.show_if_no_docstring: false– comment says “Documents objects even if they don't have docstrings”. Either set to true or fix the comment.- inherited_members: false # Include members inherited from parent classes + inherited_members: false # Do not include members inherited from parent classes - show_if_no_docstring: false # Documents objects even if they don't have docstrings + show_if_no_docstring: false # Do not document objects without docstrings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.md(1 hunks)docs/faq/contribute.md(0 hunks)docs/faq/index.md(0 hunks)docs/user-guide/mathematical-notation/index.md(1 hunks)docs/user-guide/migration-guide-v3.md(1 hunks)flixopt/calculation.py(1 hunks)flixopt/config.py(1 hunks)flixopt/core.py(1 hunks)flixopt/modeling.py(2 hunks)flixopt/results.py(2 hunks)flixopt/utils.py(2 hunks)mkdocs.yml(3 hunks)scripts/extract_changelog.py(1 hunks)
💤 Files with no reviewable changes (2)
- docs/faq/contribute.md
- docs/faq/index.md
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/modeling.py (2)
flixopt/results.py (2)
constraints(276-280)constraints(849-857)flixopt/structure.py (1)
constraints(1031-1037)
🪛 markdownlint-cli2 (0.18.1)
docs/user-guide/migration-guide-v3.md
82-82: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
83-83: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
128-128: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
129-129: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
142-142: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
147-147: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
149-149: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
150-150: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
151-151: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
152-152: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
154-154: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
155-155: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
156-156: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
157-157: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
158-158: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
163-163: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
164-164: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
197-197: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
198-198: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
199-199: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
201-201: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
202-202: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
203-203: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
204-204: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
205-205: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
206-206: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (7)
CHANGELOG.md (1)
74-74: LGTM! Helpful migration guide reference.The link to the migration guide is a valuable addition that helps users navigate breaking changes in v3.0.0.
docs/user-guide/mathematical-notation/index.md (1)
6-6: LGTM! Better explicit documentation links.Changing from directory references to explicit
index.mdfiles improves compatibility with documentation generators and makes the links more explicit.flixopt/results.py (1)
1246-1246: LGTM! Good type hint additions.Adding explicit return type annotations improves API clarity and enables better type checking without changing behavior.
Also applies to: 1402-1402
flixopt/config.py (1)
289-289: LGTM! Good type annotation.Adding the return type annotation improves type safety without changing behavior.
scripts/extract_changelog.py (1)
139-145: LGTM! Helpful navigation guidance.The added text guides users to use the navigation menu, which aligns well with the literate-nav plugin integration mentioned in the comment.
flixopt/modeling.py (1)
399-423: Confirm callers, documentation, and consistency after return type change.
- Ensure all invocations of
basic_bounds()now handle alist[linopy.constraints.Constraint]instead of the previous tuple/dict.- Verify this breaking change is recorded in the CHANGELOG.
- Check that other
BoundingPatternsmethods return types remain consistent.mkdocs.yml (1)
36-40: No change needed: API Reference path is generated at build time
Thescripts/gen_ref_pages.pyscript (via mkdocs-gen-files) writes todocs/api-reference, so the nav entry is valid.
| # By default: investment sizes are shared across scenarios, flow rates vary | ||
| # To make sizes scenario-independent: | ||
| flow_system = fx.FlowSystem( | ||
| time=timesteps, | ||
| scenarios=scenarios, | ||
| scenario_independent_sizes=True # Each scenario gets its own capacity | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
Scenario independence flag: wording likely inverted
Text says “To make sizes scenario-independent” but the comment reads “Each scenario gets its own capacity” (scenario-dependent). Verify the flag semantics and fix either the flag value or the comment.
- If True means shared across scenarios: change the comment to “Sizes shared across scenarios”.
- If True means per-scenario capacities: rename the flag in code or docs (e.g., scenario_dependent_sizes=True) or set scenario_independent_sizes=False to match wording.
🏁 Script executed:
#!/bin/bash
grep -R "scenario_independent_sizes" -n .Length of output: 4129
Fix inverted wording of scenario_independent_sizes comment
Change inline comment in docs/user-guide/migration-guide-v3.md:376 from “Each scenario gets its own capacity” to “Sizes shared across scenarios”.
🤖 Prompt for AI Agents
In docs/user-guide/migration-guide-v3.md around lines 371-378, the inline
comment on line 376 is inverted; update the comment text on the
scenario_independent_sizes argument to the requested wording by replacing “Each
scenario gets its own capacity” with “Sizes shared across scenarios” so the
comment matches the intended description.
| storage = fx.Storage( | ||
| 'storage', | ||
| charging=fx.Flow('charge', bus='electricity', size=fx.InvestParameters(effects_per_size=100, minimum_size=5)), | ||
| discharging=fx.Flow('discharge', bus='electricity', size=fx.InvestParameters(), | ||
| balanced=True, # Ensures charge_size == discharge_size | ||
| capacity_in_flow_hours=100 | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix code sample: missing parenthesis and parameter rename in Balanced Storage example
- Close the
fx.Flow('discharge', ...)call. - Use the new
effects_of_investment_per_sizeparameter (consistent with the guide).
storage = fx.Storage(
'storage',
- charging=fx.Flow('charge', bus='electricity', size=fx.InvestParameters(effects_per_size=100, minimum_size=5)),
- discharging=fx.Flow('discharge', bus='electricity', size=fx.InvestParameters(),
+ charging=fx.Flow('charge', bus='electricity', size=fx.InvestParameters(effects_of_investment_per_size=100, minimum_size=5)),
+ discharging=fx.Flow('discharge', bus='electricity', size=fx.InvestParameters()),
balanced=True, # Ensures charge_size == discharge_size
capacity_in_flow_hours=100
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| storage = fx.Storage( | |
| 'storage', | |
| charging=fx.Flow('charge', bus='electricity', size=fx.InvestParameters(effects_per_size=100, minimum_size=5)), | |
| discharging=fx.Flow('discharge', bus='electricity', size=fx.InvestParameters(), | |
| balanced=True, # Ensures charge_size == discharge_size | |
| capacity_in_flow_hours=100 | |
| ) | |
| ``` | |
| storage = fx.Storage( | |
| 'storage', | |
| charging=fx.Flow('charge', bus='electricity', size=fx.InvestParameters(effects_of_investment_per_size=100, minimum_size=5)), | |
| discharging=fx.Flow('discharge', bus='electricity', size=fx.InvestParameters()), | |
| balanced=True, # Ensures charge_size == discharge_size | |
| capacity_in_flow_hours=100 | |
| ) |
🤖 Prompt for AI Agents
In docs/user-guide/migration-guide-v3.md around lines 414 to 421 the Balanced
Storage example has an unclosed fx.Flow call and uses the old parameter name;
close the parentheses for fx.Flow('discharge', ...) and replace
InvestParameters(effects_per_size=100, minimum_size=5) with
InvestParameters(effects_of_investment_per_size=100, minimum_size=5) (or the
equivalent new parameter name) so the discharge flow call is properly closed and
the parameter name matches the rest of the guide.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
flixopt/calculation.py (1)
289-293: Docstring improved; consider adding explicit types.Nice clarification. For consistency with code, include types inline:
- aggregation (Aggregation | None): Clustered time-series data.
- aggregation_model (AggregationModel | None): Variables/constraints to equalize clusters.
- Attributes: - aggregation: Contains the clustered time series data - aggregation_model: Contains Variables and Constraints that equalize clusters of the time series data + Attributes: + aggregation (Aggregation | None): Clustered time-series data. + aggregation_model (AggregationModel | None): Variables and constraints that equalize clusters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
flixopt/calculation.py(4 hunks)flixopt/core.py(2 hunks)flixopt/utils.py(2 hunks)mkdocs.yml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- flixopt/core.py
- flixopt/utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/calculation.py (1)
flixopt/aggregation.py (4)
Aggregation(41-229)AggregationModel(288-382)AggregationParameters(232-285)do_modeling(310-340)
⏰ 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.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (4)
mkdocs.yml (2)
15-17: Migration guide addition looks good.Nice to see the dedicated v3.0 migration guide surfaced in the nav.
115-116: Verify literate-nav availability.Adding this plugin requires
mkdocs-literate-nav(or equivalent) to be installed in the docs environment and the referencedSUMMARY.mdto exist; otherwise the build will break. Please double-check both before merging.flixopt/calculation.py (2)
26-26: Import update looks good.Bringing Aggregation into the module-wide import aligns with the new wiring.
312-314: Explicit attribute typing: LGTM.Clearer lifecycle and intent; matches the updated docstring.
| self.aggregation_model = AggregationModel( | ||
| self.model, self.aggregation_parameters, self.flow_system, self.aggregation, self.components_to_clusterize | ||
| ) | ||
| self.aggregation.do_modeling() | ||
| self.aggregation_model.do_modeling() | ||
| self.durations['modeling'] = round(timeit.default_timer() - t_start, 2) |
There was a problem hiding this comment.
Blocker: components_to_clusterize may be stale after FlowSystem rebuild.
when aggregate_data_and_fix_non_binary_vars=True you rebuild self.flow_system in _perform_aggregation. Any preexisting Component instances in self.components_to_clusterize will point to the old FlowSystem and won’t match the freshly modeled self.model, causing mismatched variables or errors in AggregationModel.do_modeling().
Remap selected components to the current FlowSystem before constructing AggregationModel:
# Model the System
self.model = self.flow_system.create_model(self.normalize_weights)
self.model.do_modeling()
# Add Aggregation Submodel after modeling the rest
+ # Remap selected components to the (potentially) rebuilt FlowSystem
+ if self.components_to_clusterize:
+ labels = [c.label_full for c in self.components_to_clusterize]
+ self.components_to_clusterize = [self.flow_system.components[l] for l in labels]
self.aggregation_model = AggregationModel(
self.model, self.aggregation_parameters, self.flow_system, self.aggregation, self.components_to_clusterize
)
self.aggregation_model.do_modeling()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.aggregation_model = AggregationModel( | |
| self.model, self.aggregation_parameters, self.flow_system, self.aggregation, self.components_to_clusterize | |
| ) | |
| self.aggregation.do_modeling() | |
| self.aggregation_model.do_modeling() | |
| self.durations['modeling'] = round(timeit.default_timer() - t_start, 2) | |
| # Model the System | |
| self.model = self.flow_system.create_model(self.normalize_weights) | |
| self.model.do_modeling() | |
| # Add Aggregation Submodel after modeling the rest | |
| # Remap selected components to the (potentially) rebuilt FlowSystem | |
| if self.components_to_clusterize: | |
| labels = [c.label_full for c in self.components_to_clusterize] | |
| self.components_to_clusterize = [self.flow_system.components[l] for l in labels] | |
| self.aggregation_model = AggregationModel( | |
| self.model, self.aggregation_parameters, self.flow_system, self.aggregation, self.components_to_clusterize | |
| ) | |
| self.aggregation_model.do_modeling() | |
| self.durations['modeling'] = round(timeit.default_timer() - t_start, 2) |
🤖 Prompt for AI Agents
In flixopt/calculation.py around lines 324 to 328, self.components_to_clusterize
may reference Component instances from the old FlowSystem rebuilt earlier,
causing mismatches when passing them to AggregationModel; before constructing
AggregationModel, remap the selected components to the current self.flow_system
by replacing each component in self.components_to_clusterize with the
corresponding component instance from self.flow_system (e.g., lookup by a stable
identifier such as name or id), skip or raise if not found, and then pass the
remapped list into AggregationModel so the model and components reference the
same FlowSystem.
| - Contribute: contribute.md | ||
| - API Reference: | ||
| - api-reference/index.md | ||
| - Aggregation: api-reference/aggregation.md | ||
| - Calculation: api-reference/calculation.md | ||
| - Commons: api-reference/commons.md | ||
| - Components: api-reference/components.md | ||
| - Config: api-reference/config.md | ||
| - Core: api-reference/core.md | ||
| - Effects: api-reference/effects.md | ||
| - Elements: api-reference/elements.md | ||
| - Features: api-reference/features.md | ||
| - Flow System: api-reference/flow_system.md | ||
| - Interface: api-reference/interface.md | ||
| - IO: api-reference/io.md | ||
| - Linear Converters: api-reference/linear_converters.md | ||
| - Modeling: api-reference/modeling.md | ||
| - Network App: api-reference/network_app.md | ||
| - Plotting: api-reference/plotting.md | ||
| - Results: api-reference/results.md | ||
| - Solvers: api-reference/solvers.md | ||
| - Structure: api-reference/structure.md | ||
| - Utils: api-reference/utils.md | ||
| - API Reference: api-reference/ | ||
| - Release Notes: changelog/ |
There was a problem hiding this comment.
Point the nav entry to the actual index file.
api-reference/ isn't a valid target for MkDocs navigation and will cause the build to fail. Please link directly to the section's landing page instead.
Apply this diff:
- - API Reference: api-reference/
+ - API Reference: api-reference/index.md📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Contribute: contribute.md | |
| - API Reference: | |
| - api-reference/index.md | |
| - Aggregation: api-reference/aggregation.md | |
| - Calculation: api-reference/calculation.md | |
| - Commons: api-reference/commons.md | |
| - Components: api-reference/components.md | |
| - Config: api-reference/config.md | |
| - Core: api-reference/core.md | |
| - Effects: api-reference/effects.md | |
| - Elements: api-reference/elements.md | |
| - Features: api-reference/features.md | |
| - Flow System: api-reference/flow_system.md | |
| - Interface: api-reference/interface.md | |
| - IO: api-reference/io.md | |
| - Linear Converters: api-reference/linear_converters.md | |
| - Modeling: api-reference/modeling.md | |
| - Network App: api-reference/network_app.md | |
| - Plotting: api-reference/plotting.md | |
| - Results: api-reference/results.md | |
| - Solvers: api-reference/solvers.md | |
| - Structure: api-reference/structure.md | |
| - Utils: api-reference/utils.md | |
| - API Reference: api-reference/ | |
| - Release Notes: changelog/ | |
| - Contribute: contribute.md | |
| - API Reference: api-reference/index.md | |
| - Release Notes: changelog/ |
🤖 Prompt for AI Agents
In mkdocs.yml around lines 37 to 39, the nav entry for "API Reference" points to
a directory ("api-reference/") which MkDocs rejects; change that entry to point
to the section's landing page file (for example replace "api-reference/" with
the actual landing file path such as "api-reference/index.md" or
"api-reference/README.md"), ensure the target filename exactly matches the
existing file in the repo, and keep the other nav entries unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
flixopt/calculation.py (1)
324-327: Critical: Stale component references after FlowSystem rebuild (unresolved from previous review).The blocker issue flagged in the previous review was not addressed. When
aggregate_data_and_fix_non_binary_vars=True,_perform_aggregation()rebuildsself.flow_systemon line 384, butself.components_to_clusterizestill references Component instances from the old FlowSystem. This causes a mismatch when these components are passed toAggregationModel, leading to incorrect variable references or errors.Apply the fix suggested in the previous review:
# Model the System self.model = self.flow_system.create_model(self.normalize_weights) self.model.do_modeling() # Add Aggregation Submodel after modeling the rest + # Remap selected components to the (potentially) rebuilt FlowSystem + if self.components_to_clusterize: + labels = [c.label_full for c in self.components_to_clusterize] + self.components_to_clusterize = [self.flow_system.components[l] for l in labels] self.aggregation_model = AggregationModel( self.model, self.aggregation_parameters, self.flow_system, self.aggregation, self.components_to_clusterize ) self.aggregation_model.do_modeling()Based on past review comments.
🧹 Nitpick comments (1)
flixopt/calculation.py (1)
332-332: Remove duplicate import.
Aggregationis already imported at line 26, making this import redundant.Apply this diff:
def _perform_aggregation(self): - from .aggregation import Aggregation - t_start_agg = timeit.default_timer()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
flixopt/calculation.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/calculation.py (1)
flixopt/aggregation.py (4)
Aggregation(41-229)AggregationModel(288-382)AggregationParameters(232-285)do_modeling(310-340)
⏰ 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.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (2)
flixopt/calculation.py (2)
289-293: LGTM! Docstring accurately reflects attribute lifecycle.The updated docstring now clearly documents that
aggregationcontains the clustered time series data (Aggregation instance) andaggregation_modelcontains the modeling constraints. This addresses the previous review comment about clarifying the types and lifecycle.
312-313: LGTM! Type annotations improve code clarity.The explicit type annotations for
aggregationandaggregation_modelmake the code more maintainable and help static type checkers. This addresses the previous suggestion to add type hints.
Description
Fix docs from v3.0.0 and add a migration guide
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Documentation
Chores