Feature/332 improve docs #333
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 15 minutes and 26 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 (2)
WalkthroughDocumentation and navigation links were standardized and editorial fixes applied across user-guide pages; two new WIP parameter docs were added. Docstrings and plotting utilities received clarifications and minor robustness fixes. Two Changes
Sequence Diagram(s)(No sequence diagram provided: changes are documentation, docstrings, and minor typing edits without control-flow or feature changes.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
🧹 Nitpick comments (19)
docs/user-guide/Mathematical Notation/LinearConverter.md (1)
19-21: Typos and naming consistency ("LinearTransformer" vs "LinearConverter"; "Concersion")
- Use “LinearConverter” consistently within this page.
- Fix “Concersion” → “Conversion”.
Apply:
-where $\text a$ can be interpreted as the conversion efficiency of the **LinearTransformer**. +where $\text a$ can be interpreted as the conversion efficiency of the **LinearConverter**. -#### Piecewise Concersion factors +#### Piecewise Conversion factorsdocs/getting-started.md (1)
22-23: Fix extra quote in pip commandThere’s an extra trailing double quote.
Apply:
-pip install "flixopt[full]"" +pip install "flixopt[full]"docs/user-guide/Mathematical Notation/Effects, Penalty & Objective.md (4)
2-13: Fix typos (“occuring”, “Assiziated”, “seperatly”)Minor spelling corrections improve readability.
Apply:
-[flixopt.effects.Effect] are used to allocate things like costs, emissions, or other "effects" occuring in the system. +[flixopt.effects.Effect] are used to allocate things like costs, emissions, or other "effects" occurring in the system. -Assiziated effects could be: +Associated effects could be: -Effects are allocated seperatly for investments and operation. +Effects are allocated separately for investments and operation.
46-55: Equation reference label formattingUse eqref with the defined label.
Apply:
-The overall sum of investment shares of an Effect $e$ is given by $\eqref{Effect_invest}$ +The overall sum of investment shares of an Effect $e$ is given by $\eqref{eq:Effect_invest}$
71-73: Swap/invert descriptions for r_{x→e,inv} and r_{x→e,op}The text for “inv” vs “op” appears swapped.
Apply:
-- $\text r_{x \rightarrow e, \text{inv}}$ being the factor between the operation part of Effect $x$ and Effect $e$ -- $\text r_{x \rightarrow e, \text{op}}(\text{t}_i)$ being the factor between the invest part of Effect $x$ and Effect $e$ +- $\text r_{x \rightarrow e, \text{inv}}$ being the factor between the invest part of Effect $x$ and Effect $e$ +- $\text r_{x \rightarrow e, \text{op}}(\text{t}_i)$ being the factor between the operation part of Effect $x$ and Effect $e$
131-133: Fix “Weigted Sum” and formattingApply:
- - ... the **Weigted Sum**Method, as the chosen **Objective Effect** can incorporate other Effects. + - ... the **Weighted Sum** method, as the chosen **Objective Effect** can incorporate other Effects.docs/SUMMARY.md (1)
6-6: Optional: make API link explicit and title consistentConsider aligning with others for consistency.
Apply:
-- [API-Reference](api-reference/) +- [API Reference](api-reference/index.md)flixopt/plotting.py (8)
222-236: Document the xlabel parameter in with_plotlyDocstring misses
xlabel, which exists in the signature.Apply:
title: The title of the plot. ylabel: The label for the y-axis. + xlabel: The label for the x-axis. fig: A Plotly figure object to plot on. If not provided, a new figure will be created.
454-459: Add missing Args in heat_map_matplotlib (title/xlabel/ylabel)Keep docs in sync with signature.
Apply:
- Args: + Args: data: A DataFrame containing the data to be visualized. The index will be used for the y-axis, and columns will be used for the x-axis. The values in the DataFrame will be represented as colors in the heatmap. color_map: The colormap to use for the heatmap. Default is 'viridis'. Matplotlib supports various colormaps like 'plasma', 'inferno', 'cividis', etc. + title: The title of the plot. + xlabel: The label for the x-axis. + ylabel: The label for the y-axis. figsize: The size of the figure to create. Default is (12, 6), which results in a width of 12 inches and a height of 6 inches.
635-665: Fix typos and no-op; correct date formats in heat_map_data_from_df
- “transfrom” → “transform”
- Remove stray no-op line
- Use '%H:%M' (minutes) instead of '%H:%MM'
Apply:
- assert pd.api.types.is_datetime64_any_dtype(df.index), ( - 'The index of the Dataframe must be datetime to transfrom it properly for a heatmap plot' - ) + assert pd.api.types.is_datetime64_any_dtype(df.index), ( + 'The index of the DataFrame must be datetime to transform it properly for a heatmap plot' + ) @@ - ('D', '15min'): ('%Y-%m-%d', '%H:%MM'), # Day and hour + ('D', '15min'): ('%Y-%m-%d', '%H:%M'), # Day and minute @@ - if time_intervals[steps_per_period] > minimum_time_diff_in_min: - time_intervals[steps_per_period] + if time_intervals[steps_per_period] > minimum_time_diff_in_min:
653-661: Guard for single-timestep indices to avoid NaT -> total_seconds() error
diff().min()can be NaT; handle empty diffs.Apply:
- minimum_time_diff_in_min = df.index.to_series().diff().min().total_seconds() / 60 # Smallest time_diff in minutes + diffs = df.index.to_series().diff().dropna() + minimum_time_diff_in_min = ( + diffs.min().total_seconds() / 60 if not diffs.empty else float('inf') + )
619-626: Doc-behavior mismatch: function asserts valid combos instead of “fallback”Either implement fallback or update the docstring. Suggest updating docstring to reflect assertion.
Apply:
- If a non-valid combination of periods and steps per period is used, falls back to numerical indices + Only specific combinations of `periods` and `steps_per_period` are supported; invalid combinations raise an assertion.
679-681: Typo: “SHift” → “Shift”Apply:
- if '%w_%A' in step_format: # SHift index of strings to ensure proper sorting + if '%w_%A' in step_format: # Shift index of strings to ensure proper sorting
1325-1330: Write HTML with str(pathlib.Path) for consistencyMinor consistency with earlier usage of str().
Apply:
- elif save and not show: - fig.write_html(filename) + elif save and not show: + fig.write_html(str(filename))
433-499: Return type may be inaccurate when engine='matplotlib'Signature annotates a Plotly Figure but the matplotlib branch returns a (Figure, Axes) tuple via export_figure.
Consider changing the return type to:
- Union[plotly.graph_objs.Figure, Tuple[plt.Figure, plt.Axes]], or
- Restrict
engineto 'plotly' in this method.docs/user-guide/Mathematical Notation/Flow.md (1)
24-26: Restore cross-links and tighten phrasing.Dropping the links hurts navigation; the sentence also reads a bit clunky. Recommend re-adding anchors and simplifying.
Please verify the correct anchors/targets before merging (e.g., whether the headings resolve to
#onoffparameters/#investparameters).-This mathematical Formulation can be extended or changed when using OnOffParameters -to define the On/Off state of the Flow, or InvestParameters, -which changes the size of the Flow from a constant to an optimization variable. +This mathematical formulation can be extended by using [OnOffParameters](#onoffparameters) +to define the on/off state of the Flow, or by using [InvestParameters](#investparameters) +to change the size of the Flow from a constant to an optimization variable.flixopt/solvers.py (3)
17-22: Standardize docstring style (dataclass → “Attributes”) and clarify mip_gap meaning.Use the same section name across solvers and make the mip_gap definition precise.
- Args: - mip_gap: Solver's mip gap setting. The MIP gap describes the accepted (MILP) objective, - and the lower bound, which is the theoretically optimal solution (LP) - time_limit_seconds: Solver's time limit in seconds. - extra_options: Additional solver options. + Attributes: + mip_gap: Acceptable relative optimality gap in [0.0, 1.0]. + time_limit_seconds: Time limit in seconds. + extra_options: Additional solver options merged into `options`.
43-47: Make Gurobi docstring consistent and map names to backend params.Align section heading and note the parameter mapping.
- Args: - mip_gap: Solver's mip gap setting. The MIP gap describes the accepted (MILP) objective, - and the lower bound, which is the theoretically optimal solution (LP) - time_limit_seconds: Solver's time limit in seconds. - extra_options: Additional solver options. + Attributes: + mip_gap: Acceptable relative optimality gap in [0.0, 1.0]; mapped to Gurobi `MIPGap`. + time_limit_seconds: Time limit in seconds; mapped to Gurobi `TimeLimit`. + extra_options: Additional solver options merged into `options`.
61-69: HiGHS docstring: minor clarity and consistency nits.Keep the “Attributes” style but clarify ranges/mapping; note thread default behavior.
- Attributes: - mip_gap: Solver's mip gap setting. The MIP gap describes the accepted (MILP) objective, - and the lower bound, which is the theoretically optimal solution (LP) - time_limit_seconds: Solver's time limit in seconds. - extra_options: Additional solver options. - threads (Optional[int]): Number of threads to use. Defaults to None. + Attributes: + mip_gap: Acceptable relative optimality gap in [0.0, 1.0]; mapped to HiGHS `mip_rel_gap`. + time_limit_seconds: Time limit in seconds; mapped to HiGHS `time_limit`. + extra_options: Additional solver options merged into `options`. + threads (Optional[int]): Number of threads to use. If None, HiGHS chooses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/SUMMARY.md(1 hunks)docs/getting-started.md(1 hunks)docs/user-guide/Mathematical Notation/Effects, Penalty & Objective.md(1 hunks)docs/user-guide/Mathematical Notation/Flow.md(1 hunks)docs/user-guide/Mathematical Notation/LinearConverter.md(1 hunks)docs/user-guide/Mathematical Notation/Storage.md(1 hunks)flixopt/core.py(0 hunks)flixopt/plotting.py(1 hunks)flixopt/results.py(2 hunks)flixopt/solvers.py(3 hunks)mkdocs.yml(0 hunks)scripts/gen_ref_pages.py(1 hunks)
💤 Files with no reviewable changes (2)
- mkdocs.yml
- flixopt/core.py
🔇 Additional comments (8)
docs/user-guide/Mathematical Notation/Storage.md (1)
2-2: Link target change looks correctThe relative link to Flow.md is appropriate for this doc section.
docs/user-guide/Mathematical Notation/LinearConverter.md (1)
1-1: Good: link updated to Flow.mdMatches the pattern used elsewhere in the PR.
docs/getting-started.md (1)
41-41: Link to examples/index.md is correctConsistent with SUMMARY.md changes.
docs/SUMMARY.md (1)
3-4: Nav targets to explicit index.md look goodKeeps consistency across docs.
scripts/gen_ref_pages.py (1)
50-51: Good: point API index to the User GuideNice cross-link from API to conceptual docs.
flixopt/plotting.py (1)
520-523: Docstring additions for heat_map_plotly are correctArgs now match the function signature.
flixopt/results.py (2)
62-74: Type return annotations: good additionForward reference style is fine; aligns with docstring “Returns” section.
103-118: Type return annotations added to from_calculation: LGTMNo behavior change; improves typing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/user-guide/Mathematical Notation/Effects, Penalty & Objective.md (4)
8-12: Remove stray list marker and keep list tight.There’s a lone “-” on Line 11 that renders as an empty bullet between items.
Apply:
- - costs - given in [€/kWh]... - - ...or emissions - given in [kg/kWh]. -- + - costs — given in [€/kWh]... + - ...or emissions — given in [kg/kWh].
42-45: Use r, not a, for effect-to-effect factor.Earlier you define the cross‑effect factor as r_{x→e}; the example below uses a_{x→e}, which is inconsistent.
-In this case, the factor $\text a_{x \rightarrow e}$ is the specific CO$_2$ price in €/kg. +In this case, the factor $\text r_{x \rightarrow e}$ is the specific CO$_2$ price in €/kg.
101-103: Grammar polish.“It’s” (contraction) and “user‑defined” improve readability.
-Additionally to the user defined [Effects](#effects), a Penalty $\Phi$ is part of every FlixOpt Model. -Its used to prevent unsolvable problems and simplify troubleshooting. +Additionally to the user‑defined [Effects](#effects), a Penalty $\Phi$ is part of every FlixOpt Model. +It’s used to prevent unsolvable problems and simplify troubleshooting.
131-131: Fix list indentation to satisfy markdownlint (MD007).Remove the leading space before the bullet.
- - ... the **Weighted Sum** method, as the chosen **Objective Effect** can incorporate other Effects. +- ... the **Weighted Sum** method, as the chosen **Objective Effect** can incorporate other Effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
-
docs/SUMMARY.md(1 hunks) -
docs/getting-started.md(2 hunks) -
docs/user-guide/Mathematical Notation/Effects, Penalty & Objective.md(5 hunks) -
docs/user-guide/Mathematical Notation/Flow.md(1 hunks) -
docs/user-guide/Mathematical Notation/LinearConverter.md(2 hunks) -
flixopt/plotting.py(8 hunks) -
flixopt/solvers.py(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/SUMMARY.md
- docs/getting-started.md
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/user-guide/Mathematical Notation/LinearConverter.md
- flixopt/solvers.py
- docs/user-guide/Mathematical Notation/Flow.md
- flixopt/plotting.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/user-guide/Mathematical Notation/Effects, Penalty & Objective.md
131-131: Unordered list indentation
Expected: 0; Actual: 1
(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.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (5)
docs/user-guide/Mathematical Notation/Effects, Penalty & Objective.md (5)
2-2: Effects intro reads well.Clear and precise phrasing. No action needed.
12-12: LGTM: split of investment vs. operation clarified.Good improvement to reader understanding.
46-46: LGTM: equation reference fixed.Switch to
$\eqref{eq:Effect_invest}$ matches the label below.
116-116: Link resolves — no action required.
docs/user-guide/Mathematical Notation/Bus.md exists and matches the linked filename (case‑sensitive).
71-72: Notation swap confirmed — consistent across docs.The r_{x→e} (inv/op) notation appears only in docs/user-guide/Mathematical Notation/Effects, Penalty & Objective.md; no other Math Notation pages use a different convention.
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Documentation
Chores