-
Notifications
You must be signed in to change notification settings - Fork 9
Fix/update migration guide and changelog #411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughDocumentation and changelog updated for a v3.0.0 API redesign: effect-sharing terminology and parameter renames, FlowSystem copy-per-Calculation semantics, label-based Bus/Effect references, Calculation.do_modeling() return change, FlowSystem I/O/selection APIs documented, and mkdocs config extended with Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Config as Config (labels)
participant Calculation
participant FlowSystem
participant Results
rect rgba(0,128,96,0.07)
User->>Config: define buses/effects using string labels
end
rect rgba(0,96,192,0.06)
User->>Calculation: call Calculation.do_modeling(config)
Note right of Calculation: do_modeling() returns Calculation\naccess model via Calculation.model
Calculation->>FlowSystem: create per-Calculation FlowSystem (copy)
FlowSystem-->>Calculation: attached to Calculation.model
end
rect rgba(192,160,0,0.06)
Calculation->>Results: store results (results.flow_system available)
User->>Results: call results.flow_system.sel(...)/to_netcdf()/from_netcdf()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)docs/user-guide/migration-guide-v3.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~144-~144: There might be a mistake here.
Context: ...ct system redesigned** (no deprecation): - Terminology changes: Effect domains re...
(QB_NEW_EN)
[grammar] ~145-~145: There might be a mistake here.
Context: ...→temporal, invest/investment→periodic - **Sharing system**: The oldspecific_shar...
(QB_NEW_EN)
[grammar] ~146-~146: There might be a mistake here.
Context: ...eriodic` syntax (see 🔥 Removed section) - FlowSystem independence: FlowSystems c...
(QB_NEW_EN)
[grammar] ~155-~155: There might be a mistake here.
Context: ...ng:** - Renamed class SystemModel to FlowSystemModel - Renamed class Model to Submodel - Re...
(QB_NEW_EN)
[grammar] ~156-~156: There might be a mistake here.
Context: ...SystemModel- Renamed classModeltoSubmodel- Renamedmode` parameter in plotting met...
(QB_NEW_EN)
[grammar] ~162-~162: There might be a mistake here.
Context: ...variable: is_invested → invested in InvestmentModel - Switch tracking variables in `OnOffModel...
(QB_NEW_EN)
docs/user-guide/migration-guide-v3.md
[grammar] ~25-~25: There might be a mistake here.
Context: ... | |-----------------|-------------------|-...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...---------------------------------------| | operation | temporal | Time-varyin...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...tional costs, occuring over time) | | invest / investment | periodic |...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ...m periodic effects ``` Migration: 1. Move share definitions to the receiving ...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...Update terminology throughout your code: - Replace "operation" with "temporal" in e...
(QB_NEW_EN)
[grammar] ~218-~218: There might be a mistake here.
Context: ...Plotting:** mode parameter renamed to style - Class names: SystemModel → `FlowSyst...
(QB_NEW_EN)
[grammar] ~219-~219: There might be a mistake here.
Context: ...emModel→FlowSystemModel, Model→Submodel` - Logging: Disabled by default (enable w...
(QB_NEW_EN)
[grammar] ~228-~228: There might be a mistake here.
Context: ...rk) ### InvestParameters | Old | New | |-----|-----| | fix_effects | `effects...
(QB_NEW_EN)
[grammar] ~229-~229: There might be a mistake here.
Context: ...tParameters | Old | New | |-----|-----| | fix_effects | `effects_of_investment...
(QB_NEW_EN)
[grammar] ~230-~230: There might be a mistake here.
Context: ...fix_effects|effects_of_investment| |specific_effects|effects_of_inves...
(QB_NEW_EN)
[grammar] ~231-~231: There might be a mistake here.
Context: ...ts|effects_of_investment_per_size| |divest_effects|effects_of_retirem...
(QB_NEW_EN)
[grammar] ~232-~232: There might be a mistake here.
Context: ...est_effects|effects_of_retirement| |piecewise_effects|piecewise_effec...
(QB_NEW_EN)
[grammar] ~237-~237: There might be a mistake here.
Context: ...investment| ### Effect | Old | New | |-----|-----| |minimum_investment|...
(QB_NEW_EN)
[grammar] ~238-~238: There might be a mistake here.
Context: ... ### Effect | Old | New | |-----|-----| | minimum_investment | `minimum_period...
(QB_NEW_EN)
[grammar] ~239-~239: There might be a mistake here.
Context: ...nimum_investment|minimum_periodic| |maximum_investment|maximum_period...
(QB_NEW_EN)
[grammar] ~240-~240: There might be a mistake here.
Context: ...ximum_investment|maximum_periodic| |minimum_operation|minimum_tempora...
(QB_NEW_EN)
[grammar] ~241-~241: There might be a mistake here.
Context: ...inimum_operation|minimum_temporal| |maximum_operation|maximum_tempora...
(QB_NEW_EN)
[grammar] ~242-~242: There might be a mistake here.
Context: ...aximum_operation|maximum_temporal| |minimum_operation_per_hour|minimu...
(QB_NEW_EN)
[grammar] ~243-~243: There might be a mistake here.
Context: ...eration_per_hour|minimum_per_hour| |maximum_operation_per_hour|maximu...
(QB_NEW_EN)
[grammar] ~248-~248: There might be a mistake here.
Context: ...our| ### SourceAndSink | Old | New | |-----|-----| |source|outputs` | |...
(QB_NEW_EN)
[grammar] ~249-~249: There might be a mistake here.
Context: ...urceAndSink | Old | New | |-----|-----| | source | outputs | | sink | `inp...
(QB_NEW_EN)
[grammar] ~250-~250: There might be a mistake here.
Context: ...| |-----|-----| | source | outputs | | sink | inputs | | `prevent_simulta...
(QB_NEW_EN)
[grammar] ~251-~251: There might be a mistake here.
Context: ...rce|outputs| |sink|inputs| |prevent_simultaneous_sink_and_source`...
(QB_NEW_EN)
[grammar] ~256-~256: There might be a mistake here.
Context: ...es| ### TimeSeriesData | Old | New | |-----|-----| |agg_group|aggregati...
(QB_NEW_EN)
[grammar] ~257-~257: There might be a mistake here.
Context: ...eSeriesData | Old | New | |-----|-----| | agg_group | aggregation_group | | ...
(QB_NEW_EN)
[grammar] ~258-~258: There might be a mistake here.
Context: ...-| | agg_group | aggregation_group | | agg_weight | aggregation_weight | ...
(QB_NEW_EN)
[grammar] ~333-~333: There might be a mistake here.
Context: ...*"Effect share parameters not working"** → Move shares to receiving effect using ...
(QB_NEW_EN)
[grammar] ~336-~336: There might be a mistake here.
Context: ...age charge state has wrong dimensions"** → Remove extra timestep; use `relative_m...
(QB_NEW_EN)
[grammar] ~339-~339: There might be a mistake here.
Context: ... **"KeyError when accessing results"** → Update variable names: -is_investe...
(QB_NEW_EN)
[grammar] ~348-~348: There might be a mistake here.
Context: ...tal→Effect **"No logging output"** → Enable explicitly:fx.CONFIG.Logging....
(QB_NEW_EN)
[grammar] ~355-~355: There might be a mistake here.
Context: ...--- ## Migration Checklist Critical: - [ ] Update flixopt: `pip install --upgra...
(QB_NEW_EN)
[grammar] ~356-~356: There might be a mistake here.
Context: ...st Critical: - [ ] Update flixopt: pip install --upgrade flixopt - [ ] Update effect sharing syntax - [ ] U...
(QB_NEW_EN)
[grammar] ~357-~357: There might be a mistake here.
Context: ...xopt` - [ ] Update effect sharing syntax - [ ] Update result variable names - [ ] R...
(QB_NEW_EN)
[grammar] ~358-~358: There might be a mistake here.
Context: ...yntax - [ ] Update result variable names - [ ] Replace object assignments with stri...
(QB_NEW_EN)
[grammar] ~359-~359: There might be a mistake here.
Context: ...ce object assignments with string labels - [ ] Fix storage charge state arrays - [ ...
(QB_NEW_EN)
[grammar] ~360-~360: There might be a mistake here.
Context: ...ls - [ ] Fix storage charge state arrays - [ ] Update do_modeling() usage if need...
(QB_NEW_EN)
[grammar] ~361-~361: There might be a mistake here.
Context: ...] Update do_modeling() usage if needed - [ ] Rename plotting mode → style - [...
(QB_NEW_EN)
[grammar] ~362-~362: There might be a mistake here.
Context: ...f needed - [ ] Rename plotting mode → style - [ ] Enable logging if needed **Recommen...
(QB_NEW_EN)
[grammar] ~365-~365: There might be a mistake here.
Context: ...Enable logging if needed Recommended: - [ ] Update deprecated parameter names - ...
(QB_NEW_EN)
[grammar] ~369-~369: There might be a mistake here.
Context: ...oughly and validate results Optional: - [ ] Explore new features (periods, scena...
(QB_NEW_EN)
[grammar] ~372-~372: There might be a mistake here.
Context: ...rage) Welcome to flixopt v3.0.0! 🎉 --- ## Resources - Docs: https://flixopt.g...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
docs/user-guide/migration-guide-v3.md
36-36: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
46-46: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
86-86: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
104-104: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
130-130: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
137-137: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
146-146: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
154-154: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
168-168: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
195-195: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
205-205: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
267-267: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
274-274: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
341-341: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
342-342: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
343-343: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
344-344: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
345-345: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
346-346: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
372-372: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
377-377: Bare URL used
(MD034, no-bare-urls)
378-378: Bare URL used
(MD034, no-bare-urls)
379-379: Bare URL used
(MD034, no-bare-urls)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/user-guide/migration-guide-v3.md(1 hunks)mkdocs.yml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/user-guide/migration-guide-v3.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...thoroughly. --- ## 💥 Breaking Changes ### Effect System Redesign Terminology chan...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...()` | --- ## 🗑️ Deprecated Parameters ??? abstract "InvestParameters" | O...
(QB_NEW_EN)
[grammar] ~206-~206: There might be a mistake here.
Context: ... 80% charged | --- ## 🔧 Common Issues | Issue | Solution | |-------|----------...
(QB_NEW_EN)
[style] ~233-~233: Using many exclamation marks might seem excessive (in this case: 10 exclamation marks for a text that’s 4509 characters long)
Context: ... !!! success "Welcome to flixopt v3.0.0! 🎉"
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
docs/user-guide/migration-guide-v3.md
7-7: Link fragments should be valid
(MD051, link-fragments)
7-7: Link fragments should be valid
(MD051, link-fragments)
48-48: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
49-49: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
50-50: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
51-51: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
52-52: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
53-53: Table column count
Expected: 3; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
54-54: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
211-211: Link fragments should be valid
(MD051, link-fragments)
224-224: Link fragments should be valid
(MD051, link-fragments)
224-224: Link fragments should be valid
(MD051, link-fragments)
226-226: Link fragments should be valid
(MD051, link-fragments)
⏰ 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)
| | Investment | `is_invested` | `invested` | | ||
| | Switching | `switch_on` | `switch|on` | | ||
| | Switching | `switch_off` | `switch|off` | | ||
| | Switching | `switch_on_nr` | `switch|count` | | ||
| | Effects | `Effect(invest)|total` | `Effect(periodic)` | | ||
| | Effects | `Effect(operation)|total` | `Effect(temporal)` | | ||
| | Effects | `Effect(operation)|total_per_timestep` | `Effect(temporal)|per_timestep` | | ||
| | Effects | `Effect|total` | `Effect` | | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escape | in table cells to keep column structure intact.
The “Variable Names” table still renders with misaligned columns because the literal | characters inside the new names aren’t escaped, so Markdown treats them as extra separators (see MD056 reports and broken layout in preview). Escape the pipes (or wrap with <code> tags) so the migration guide stays copy/paste-safe.
-| Switching | `switch_on` | `switch|on` |
-| Switching | `switch_off` | `switch|off` |
-| Switching | `switch_on_nr` | `switch|count` |
-| Effects | `Effect(invest)|total` | `Effect(periodic)` |
-| Effects | `Effect(operation)|total` | `Effect(temporal)` |
-| Effects | `Effect(operation)|total_per_timestep` | `Effect(temporal)|per_timestep` |
-| Effects | `Effect|total` | `Effect` |
+| Switching | `switch_on` | `switch\|on` |
+| Switching | `switch_off` | `switch\|off` |
+| Switching | `switch_on_nr` | `switch\|count` |
+| Effects | `Effect(invest)\|total` | `Effect(periodic)` |
+| Effects | `Effect(operation)\|total` | `Effect(temporal)` |
+| Effects | `Effect(operation)\|total_per_timestep` | `Effect(temporal)\|per_timestep` |
+| Effects | `Effect\|total` | `Effect` |📝 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.
| | Investment | `is_invested` | `invested` | | |
| | Switching | `switch_on` | `switch|on` | | |
| | Switching | `switch_off` | `switch|off` | | |
| | Switching | `switch_on_nr` | `switch|count` | | |
| | Effects | `Effect(invest)|total` | `Effect(periodic)` | | |
| | Effects | `Effect(operation)|total` | `Effect(temporal)` | | |
| | Effects | `Effect(operation)|total_per_timestep` | `Effect(temporal)|per_timestep` | | |
| | Effects | `Effect|total` | `Effect` | | |
| | Investment | `is_invested` | `invested` | | |
| | Switching | `switch_on` | `switch\|on` | | |
| | Switching | `switch_off` | `switch\|off` | | |
| | Switching | `switch_on_nr` | `switch\|count` | | |
| | Effects | `Effect(invest)\|total` | `Effect(periodic)` | | |
| | Effects | `Effect(operation)\|total` | `Effect(temporal)` | | |
| | Effects | `Effect(operation)\|total_per_timestep` | `Effect(temporal)\|per_timestep` | | |
| | Effects | `Effect\|total` | `Effect` | |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
48-48: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
49-49: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
50-50: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
51-51: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
52-52: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
53-53: Table column count
Expected: 3; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
54-54: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
In docs/user-guide/migration-guide-v3.md around lines 47 to 55, the table
columns are broken because literal pipe characters in the "new names" column are
unescaped; update each affected cell to either escape the pipe as \| or wrap the
value in inline code/HTML (e.g. <code>...</code>) so the pipes are treated as
literal characters (e.g. change switch|on → switch\|on or
`<code>switch|on</code>` for all occurrences: switch|on, switch|off,
switch|count, Effect(invest)|total, Effect(operation)|total,
Effect(operation)|total_per_timestep, Effect|total, etc.) and re-run the
Markdown linter to ensure MD056 no longer reports errors.
Description
Update the Migration guide and improve the release notes for missed changes
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Breaking Changes
Documentation
Chores