Feature/v3/feature/367 rename year dimension to period 2

[FBumann]

Description

Brief description of the changes in this PR.

Type of Change

• Bug fix
• New feature
• Documentation update
• Code refactoring

Related Issues

#367

Testing

• I have tested my changes
• Existing tests still pass

Checklist

• My code follows the project style
• I have updated documentation if needed
• I have added tests for new functionality (if applicable)

Summary by CodeRabbit

• New Features

  • Introduced “period/periodic” terminology across the app and results, aligning all time-aggregated data and effect modes.

• Refactor

  • Public API updates: FlowSystem now uses periods instead of years.
  • Effect parameters and shares renamed (e.g., minimum/maximum_periodic; share_from_periodic).
  • Results modes updated from nontemporal to periodic (e.g., get_effect_shares accepts 'temporal' or 'periodic').

• Documentation

  • Changelog updated to reflect new parameter names and terminology.

• Examples

  • Example scenarios updated to use periods and show periodic costs.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Renames “nontemporal/year” to “periodic/period” across the codebase. Public APIs change accordingly (e.g., FlowSystem years→periods; Effect parameters and modes switch to periodic). Coordinates/dims rekeyed to period. Results/calculation keys updated to periodic. One control-flow tweak in FlowSystemModel.weights consolidates weight fitting/normalization.

Changes

Cohort / File(s)	Summary
Core terminology and types flixopt/core.py	Introduces PeriodicData/User types; replaces 'year' with 'period' in FlowSystemDimensions; adds backward-compat aliases for NonTemporal*.
FlowSystem and modeling infrastructure flixopt/flow_system.py, flixopt/modeling.py, flixopt/structure.py	FlowSystem API: years→periods; selection and coords renamed to period; serialization updated. Modeling default coord in a helper switches to period. FlowSystemModel.weights path simplified to fit-and-normalize with period dims when missing.
Effects domain rename flixopt/effects.py	Renames nontemporal→periodic throughout Effect API, modeling, shares, types, and dims (year→period). Adds compatibility aliases for NonTemporalEffects*. Updates labels/short_names and cycle/share checks.
Components, interface, elements, features flixopt/components.py, flixopt/interface.py, flixopt/elements.py, flixopt/features.py	Replace NonTemporalDataUser with PeriodicDataUser; update InvestParameters and storage-related fields to periodic types; switch all year-based dims/coords/messages to period; adjust piecewise/share targets to 'periodic'.
Results and calculation flixopt/results.py, flixopt/calculation.py	Results modes/keys updated from nontemporal to periodic; APIs now accept Literal['temporal','periodic']; datasets and aggregation reflect 'period'; calculation main_results maps submodel key to 'periodic'.
Examples examples/04_Scenarios/scenario_example.py, examples/05_Two-stage-optimization/two_stage_optimization.py	FlowSystem constructor updated to periods; result selection column renamed costs(periodic).
Tests tests/test_effect.py, tests/test_flow.py, tests/test_integration.py, tests/test_storage.py, tests/conftest.py	Update expectations, coords, and labels from year/nontemporal to period/periodic; fixtures now provide period-based configurations.
Changelog CHANGELOG.md	Documents renames: share_from_nontemporal→share_from_periodic; min/max mappings; removed parameter mapping note.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant FS as FlowSystem
  participant ECM as EffectCollectionModel
  participant SOL as Solver
  participant RES as Results

  U->>FS: build(model with period coords)
  FS->>ECM: add Effect(temporal, periodic)
  note right of ECM: total = temporal + periodic
  ECM->>SOL: optimize (period/time/scenario dims)
  SOL-->>ECM: solution vars
  ECM-->>RES: effects dataset (temporal/periodic/total)
  RES-->>U: costs(periodic), shares, totals

Loading

sequenceDiagram
  autonumber
  participant FSM as FlowSystemModel
  participant FIT as fit_to_model_coords

  FSM->>FSM: get weights
  alt weights provided
    FSM-->>FSM: return provided weights
  else weights missing
    FSM->>FIT: fit weights over ('period','scenario')
    FIT-->>FSM: fitted weights
    FSM-->>FSM: normalize (divide by sum)
  end
  FSM-->>FSM: use weights in modeling

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Feature/308 rename effect domains #360 — Earlier rename of invest/operation to nontemporal/temporal in Effects; this PR continues by renaming nontemporal→periodic in the same areas.
• Feature/v3/feature/305 rename specific share to other effects to specific share from effect #366 — Changes effect sharing parameters (specific_share_to_other_effects_* to share_from_*), overlapping with this PR’s share_from_periodic updates.
• Feature/v3/update #352 — Touches FlowSystem constructor/coords and interfaces; overlaps with periods-based API changes here.

Suggested labels

v3.0.0

Suggested reviewers

• baumbude
• PStange

Poem

Little paws tap-tap through time’s new thread,
From years to periods the carrots spread.
Periodic crumbs, no nontemporal chase,
Effects align, in a tidy place.
Weights now fitted, normalized cheer—
Hop! The model’s clearer here. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Title Check	⚠️ Warning	The pull request title includes branch metadata (“Feature/v3/feature/367”) and an extraneous “2,” which adds noise and detracts from clearly conveying the main change; while it does mention renaming the year dimension to period, the title is overly verbose and not phrased as a concise, descriptive sentence.	Please simplify the title to focus on the key change, for example “Rename year dimension to period,” and remove branch identifiers and the trailing “2” to make it clear and concise.
Description Check	⚠️ Warning	The pull request description remains unchanged from the template, with placeholder text in the Description section and unchecked Testing boxes, providing no actual summary of the changes or confirmation that tests were run and passed.	Please fill in the Description section with a brief summary of the renaming work performed, update the Testing section to indicate which tests were run and their outcomes, and ensure that all relevant template sections accurately reflect the PR’s content.
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.57% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[FBumann]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

flixopt/components.py (1)

833-839: Discharge efficiency applied in the wrong direction (physics + doc mismatch)

Docstring states charge[t+1] subtracts discharge_flow / eta_discharge. Implementation multiplies by eta_discharge, which increases effective storage energy for η<1.

Apply discharge efficiency as a divisor:

-            - discharge_rate * eff_discharge * hours_per_step,
+            - discharge_rate * hours_per_step / xr.where(eff_discharge != 0, eff_discharge, np.nan),

If xarray import scope disallows np.nan here, precompute a safe divisor. Update tests that hard-code the previous (incorrect) formulation (see tests/test_storage.py lossy case).

I can adjust the affected tests and add a regression test for η_discharge handling. Want a patch?

flixopt/flow_system.py (1)

273-283: from_dataset passes an unexpected constructor argument.

FlowSystem.init has no periods_of_last_period parameter, but from_dataset passes it. This raises TypeError during restore.

Apply one of the fixes (A: remove arg, B: add optional param):

-            periods_of_last_period=reference_structure.get('periods_of_last_period'),

Or, if needed elsewhere, add it to init with default and store in attrs. Choose A unless you use it later.

</blockquote></details>
<details>
<summary>flixopt/effects.py (1)</summary><blockquote>

`171-179`: **Type hints for min/max fields should be Data, not Effects mappings.**

minimum_temporal/maximum_temporal/minimum_periodic/maximum_periodic should accept PeriodicDataUser (or Scalar), not PeriodicEffectsUser (which maps effect→value). This will confuse type checkers and readers.


```diff
-        minimum_temporal: PeriodicEffectsUser | None = None,
-        maximum_temporal: PeriodicEffectsUser | None = None,
-        minimum_periodic: PeriodicEffectsUser | None = None,
-        maximum_periodic: PeriodicEffectsUser | None = None,
+        minimum_temporal: PeriodicDataUser | None = None,
+        maximum_temporal: PeriodicDataUser | None = None,
+        minimum_periodic: PeriodicDataUser | None = None,
+        maximum_periodic: PeriodicDataUser | None = None,

🧹 Nitpick comments (8)

CHANGELOG.md (1)

125-126: Clarify deprecation vs breaking/removal for renamed Effect params.

• Lines 125–126 are under “Deprecated” but look like immediate renames that will break users if old names stop working.
• Line 135 states the share parameter change happened WITHOUT DEPRECATION.

To avoid ambiguity, either:

• Move truly breaking renames to “Breaking Changes”/“Removed”, or
• Explicitly state old names are still accepted and scheduled for removal in version X.Y with a deprecation window.

Proposed wording tweak:

-### 🗑️ Deprecated
-* Renamed `Effect` parameters:
+### 🗑️ Deprecated
+* Effect parameters deprecated (old names still accepted until 3.0, then removed):
   - `minimum_investment` → `minimum_periodic`
   - `maximum_investment` → `maximum_periodic`

And under “🔥 Removed” add the explicit note that the specific_share_to_other_effects_* → share_from_* happened without deprecation (already present).

Also applies to: 135-135

tests/conftest.py (1)

45-63: Fixture updates to periods/scenarios — LGTM. Consider adding time+scenarios (no periods).

A fourth param set with periods=None and scenarios non‑None would increase coverage for logging/summary paths that assume periodic keys.

     params=[
         {
             'timesteps': pd.date_range('2020-01-01', periods=10, freq='h', name='time'),
             'periods': None,
             'scenarios': None,
         },
+        {
+            'timesteps': pd.date_range('2020-01-01', periods=10, freq='h', name='time'),
+            'periods': None,
+            'scenarios': pd.Index(['A', 'B'], name='scenario'),
+        },
         {
             'timesteps': pd.date_range('2020-01-01', periods=10, freq='h', name='time'),
             'periods': pd.Index([2020, 2030, 2040], name='period'),
             'scenarios': None,
         },
         {
             'timesteps': pd.date_range('2020-01-01', periods=10, freq='h', name='time'),
             'periods': pd.Index([2020, 2030, 2040], name='period'),
             'scenarios': pd.Index(['A', 'B'], name='scenario'),
         },
     ],
-    ids=['time_only', 'time+periods', 'time+periods+scenarios'],
+    ids=['time_only', 'time+scenarios', 'time+periods', 'time+periods+scenarios'],

flixopt/modeling.py (1)

661-673: Infer coord from level_variable.dims to reduce coupling.
To support custom time-dimension names and avoid silent mismatches, auto-detect the time-like dim when the default 'period' isn’t present:

@@ def link_changes_to_level_with_binaries(...):
-        coord: str = 'period',
+        coord: str = 'period',
@@
-        # 1. Initial period: level[0] - initial_level =  increase[0] - decrease[0]
+        # Auto-infer time dimension if needed
+        if coord not in level_variable.dims:
+            candidates = [d for d in ('period', 'time') if d in level_variable.dims]
+            coord = candidates[0] if candidates else level_variable.dims[0]
+        # 1. Initial period: level[0] - initial_level = increase[0] - decrease[0]

flixopt/core.py (1)

626-629: Backward-compat aliases present; consider deprecation messaging.

If you plan to drop NonTemporal* soon, add a DeprecationWarning on access or document the removal timeline (e.g., in CHANGELOG).

tests/test_effect.py (1)

65-66: Zeroing periodic totals in “minimal/bounds” tests may mask real shares.

Both tests force periodic totals to 0. If future components/effects provide periodic shares, these asserts will fail unexpectedly. Prefer asserting presence/shape or non-negativity unless explicitly testing zero-share scenarios.

-        assert_conequal(model.constraints['Effect1(periodic)'], model.variables['Effect1(periodic)'] == 0)
+        # Assert variable exists and has expected coords; value depends on model inputs
+        assert 'Effect1(periodic)' in model.variables
+        assert set(model.variables['Effect1(periodic)'].dims) == set(model.get_coords(['period','scenario']).keys())

Also applies to: 139-142

flixopt/results.py (1)

621-676: Effects dataset construction: strengthen naming and guard template.

• On scalar-NaN paths, use xr.full_like with dtype=float (good). After concatenation, ensure var names are consistent.

-            ds[effect] = xr.concat(component_arrays, dim='component', coords='minimal')
+            ds[effect] = xr.concat(component_arrays, dim='component', coords='minimal').rename(effect)

Also consider raising on mismatch instead of only logging critical; tests will catch regressions earlier.

-            if not np.allclose(computed.values, found.fillna(0).values):
-                logger.critical(
+            if not np.allclose(computed.values, found.fillna(0).values):
+                raise ValueError(
                     f'Results for {effect}({mode}) in effects_dataset doesnt match {label}\n{computed=}\n, {found=}'
                 )

flixopt/flow_system.py (1)

429-441: Weights normalization: consider normalizing instead of warning.

Currently logs a warning but proceeds. Optional auto-normalization would avoid scaling surprises.

         if self.weights is not None:
             total = float(self.weights.sum().item())
             if not np.isclose(total, 1.0, atol=1e-12):
-                logger.warning(
-                    'Scenario weights are not normalized to 1. Normalizing to 1 is recommended for a better scaled model. '
-                    f'Sum of weights={total}'
-                )
+                logger.warning('Scenario weights sum to %s. Auto-normalizing to 1.', total)
+                self.weights = self.weights / total

flixopt/effects.py (1)

769-837: BFS accumulation in conversion paths: confirm intended sum semantics.

You sum parallel paths’ factors. If shares are meant to combine additively, fine; if they should multiply or choose shortest-path, document it. Consider casting all factors to consistent dtype early.

-    result = {}
+    result: dict[tuple[str,str], xr.DataArray | np.ndarray | float | int] = {}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bc0a262 and a555cb5.

📒 Files selected for processing (19)

• CHANGELOG.md (3 hunks)
• examples/04_Scenarios/scenario_example.py (2 hunks)
• examples/05_Two-stage-optimization/two_stage_optimization.py (1 hunks)
• flixopt/calculation.py (1 hunks)
• flixopt/components.py (3 hunks)
• flixopt/core.py (2 hunks)
• flixopt/effects.py (18 hunks)
• flixopt/elements.py (4 hunks)
• flixopt/features.py (10 hunks)
• flixopt/flow_system.py (13 hunks)
• flixopt/interface.py (6 hunks)
• flixopt/modeling.py (1 hunks)
• flixopt/results.py (17 hunks)
• flixopt/structure.py (1 hunks)
• tests/conftest.py (1 hunks)
• tests/test_effect.py (12 hunks)
• tests/test_flow.py (16 hunks)
• tests/test_integration.py (2 hunks)
• tests/test_storage.py (1 hunks)

⏰ 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.11)
• GitHub Check: test (3.10)

🔇 Additional comments (32)

CHANGELOG.md (1)

58-59: Example aligns with new “periodic” naming — LGTM.

Snippet correctly uses share_from_periodic alongside share_from_temporal.

examples/05_Two-stage-optimization/two_stage_optimization.py (1)

149-151: Variables verified: costs(periodic) and costs(temporal) are present; no costs(nontemporal) risk
The selection keys 'costs', 'costs(periodic)', and 'costs(temporal)' all exist in two_stage_optimization.py, and there is no longer any reference to 'costs(nontemporal)'. No KeyError is expected.

flixopt/calculation.py (1)

106-115: Periodic submodel always present — no guard needed
EffectModel._do_modeling unconditionally creates self.periodic for every effect, so effect.submodel.periodic is always defined and won’t raise an AttributeError.

Likely an incorrect or invalid review comment.

tests/test_storage.py (1)

291-296: Rename to period/scenario looks correct

Investment variables now use coords over ['period','scenario'], matching the new API.

tests/test_integration.py (3)

131-136: Updated to periodic keys

Expectation strings updated to costs(periodic). Aligns with API.

---

137-141: Updated storage costs key

Speicher->costs(periodic) reference matches rename.

---

148-152: CO2(periodic) key rename

Matches the new output naming.

examples/04_Scenarios/scenario_example.py (2)

14-15: Introduce periods Index

Using periods=pd.Index([...]) is correct for the new API.

---

24-25: FlowSystem weights broadcasting confirmed: 1D scenario weights are aligned to both period and scenario dims and broadcast across periods as intended.

flixopt/components.py (2)

374-387: Storage ctor: PeriodicDataUser types

Parameter typing updated for period-based data. Looks consistent with the rename.

---

445-468: Ensure final relative charge-state dims align with temporal concat
The hard-coded dims=['period','scenario'] for relative_minimum_final_charge_state and relative_maximum_final_charge_state may not match the time-based dims of the original relative_*_charge_state, causing xr.concat(..., dim='time') to misalign or fail. Verify and update the dims passed to flow_system.fit_to_model_coords for these final states so they match the existing temporal dims (i.e., drop 'time' from the original dims) before concatenation.

tests/test_flow.py (4)

30-32: LGTM on period-dimension migration in tests.

Assertions now consistently use model.get_coords(['period','scenario']) and '(periodic)' effect channels; they align with implementation changes in features/elements.

Also applies to: 69-70, 182-183, 243-248, 315-323, 388-392, 471-478, 483-487, 508-513, 566-568, 1015-1016, 1060-1061, 1132-1133, 1159-1160, 1231-1232, 440-441

---

471-478: Nice coverage for periodic investment/divest effects.

Checks for costs/CO2(periodic) variables and constraints look correct and validate both fixed/specific and divest cases.

Also applies to: 508-513, 1015-1016, 1060-1061

---

243-248: Good consistency on size/on_hours_total bounds over ['period','scenario'].

Bounds for size, on_hours_total, and switch|count match the new periodic API.

Also applies to: 317-323, 390-392, 441-441, 567-568, 1132-1133, 1159-1160, 1231-1232

---

1-1329: No stale ‘year’ dimensions found outside documentation. Repo-wide scan returned matches only in CHANGELOG, plotting docstrings, and project metadata.

flixopt/features.py (4)

61-62: Periodic coords and targets applied correctly.

Size/is_invested now over ['period','scenario']; investment effects target='periodic'. Matches tests.

Also applies to: 67-68, 100-101, 110-111, 117-118

---

173-179: Use None instead of np.inf for unbounded upper.

np.inf as an upper bound can inflate coefficients and degrade solver stability. Prefer None to indicate “unbounded” (consistent with FlowModel usage).
[ suggest_recommended_refactor ]
Apply this diff:

-            bounds=(
-                self.parameters.on_hours_total_min if self.parameters.on_hours_total_min is not None else 0,
-                self.parameters.on_hours_total_max if self.parameters.on_hours_total_max is not None else np.inf,
-            ),  # TODO: self._model.hours_per_step.sum('time').item() + self._get_previous_on_duration())
+            bounds=(
+                self.parameters.on_hours_total_min if self.parameters.on_hours_total_min is not None else 0,
+                self.parameters.on_hours_total_max,  # None → unbounded upper
+            ),  # TODO: self._model.hours_per_step.sum('time').item() + self._get_previous_on_duration())

---

328-346: PieceModel dims switch to ('time','period','scenario') / ('period','scenario') looks correct.

Matches the tests and periodic API.

---

570-572: Good guard on 'period' in ShareAllocationModel.add_share.

Prevents accidental period-dim shares where the model wasn’t created with a period dim.

flixopt/elements.py (3)

432-443: Periodic migration in Flow.transform_data looks consistent.

flow_hours_total*, load_factor*, and size now fitted over ['period','scenario']; matches tests.

Also applies to: 450-451

---

533-535: Correct: total_flow_hours tracked over ['period','scenario'].

Aligns with assertions in tests and with OnOff on_hours_total handling.

---

319-321: Doc nit: “Maximum 200 starts per period” update is accurate.

Comment change reflects new nomenclature.

flixopt/interface.py (3)

17-19: API/type migration to Periodic and period dims looks good.*

InvestParameters and effects now use PeriodicData/User and map to ['period','scenario']; Piece.transform_data handles time vs non-time pieces correctly.

Also applies to: 856-870, 879-906, 898-906, 880-892

---

1143-1150: OnOffParameters periodic bounds mapping matches features/tests.

on_hours_total_{min,max} and switch_on_total_max now bound over ['period','scenario']; aligns with OnOffModel and tests.

Also applies to: 1146-1147

---

1-1180: TemporalEffectsUser and PeriodicEffectsUser exist and are exported in flixopt/effects.py. No further changes required.

flixopt/flow_system.py (1)

744-752: coords property: good—ensure period/scenario are present only when provided.

LGTM; dynamic inclusion aligns with API rename.

flixopt/effects.py (4)

409-421: Dim choices for share factors: correct after rename.

Temporal shares: ('time','period','scenario'); Periodic: ('period','scenario'). Looks consistent. Nice.

---

487-496: Total = temporal + periodic: clear and matches tests.

Modeling equality and coords use ['period','scenario']—LGTM.

---

586-603: Cycle detection and unknown-source validation are solid.

Good safeguards for share graphs; error messages are explicit.

---

915-917: Compat aliases are helpful.

Thanks for keeping NonTemporal* aliases.

tests/test_effect.py (1)

202-221: Add dims alignment assertion for periodic share variable

Insert an assertion after the existing assert_conequal checks in tests/test_effect.py to verify that the periodic share variable carries the expected dimensions:

assert model.variables['Effect1(periodic)->Effect2(periodic)'].dims == ('period', 'scenario')

Manual verification of dims in your test environment is required to ensure this guard catches any coordinate misalignments.

flixopt/results.py (1)

356-364: ‘period’ dimension is present and consumers use name-based dims
The transpose with missing_dims='ignore' includes period as a dataset coord, and all downstream code (including tests) reference dims by name rather than by position, so no further action is needed.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Doc/type mismatch for PeriodicData.

Type alias sets PeriodicData = xr.DataArray, but the docstring says “Can be a Scalar or an xr.DataArray.” Clarify one of:

• If only DataArray is intended, update docstring.
• If scalars are allowed, change the alias to Scalar | xr.DataArray.

🤖 Prompt for AI Agents

In flixopt/core.py around lines 20 to 27, the PeriodicData type alias is
declared as xr.DataArray but the docstring says it can be a Scalar or an
xr.DataArray; update one to match the other: either change the docstring to
state it is strictly an xr.DataArray, or change the type alias to include Scalar
(e.g., PeriodicData = Scalar | xr.DataArray) so the type annotation and
docstring are consistent; make the chosen fix and update the docstring
accordingly.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Bug: zero_point coords ignore 'time' when as_time_series=True and drop all coords when False.

For time-series piecewise, zero_point must include 'time' (and period, scenario); for non-time-series, it should include ['period','scenario']. Current conditional inverts this.

Apply this fix:

-                self.zero_point = self.add_variables(
-                    coords=self._model.get_coords(('period', 'scenario') if self._as_time_series else None),
-                    binary=True,
-                    short_name='zero_point',
-                )
+                dims = ('time', 'period', 'scenario') if self._as_time_series else ('period', 'scenario')
+                self.zero_point = self.add_variables(
+                    coords=self._model.get_coords(dims),
+                    binary=True,
+                    short_name='zero_point',
+                )

This aligns zero_point dims with inside_piece/lambda variables and avoids broadcasting errors in single_segment constraints.

📝 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.

Suggested change

	self.zero_point = self.add_variables(
	coords=self._model.get_coords(('year', 'scenario') if self._as_time_series else None),
	coords=self._model.get_coords(('period', 'scenario') if self._as_time_series else None),
	binary=True,
	short_name='zero_point',
	)
	# ensure zero_point has the right dims for time-series vs. non-time-series
	dims = ('time', 'period', 'scenario') if self._as_time_series else ('period', 'scenario')
	self.zero_point = self.add_variables(
	coords=self._model.get_coords(dims),
	binary=True,
	short_name='zero_point',
	)

🤖 Prompt for AI Agents

In flixopt/features.py around lines 419–423, the conditional that builds coords
for zero_point is inverted: when self._as_time_series is True zero_point must
include time (i.e., period, scenario, time) and when False it should include
only period and scenario; change the conditional so get_coords receives
('period','scenario','time') if self._as_time_series else ('period','scenario')
(do not pass None), matching the dims used by inside_piece/lambda variables to
prevent broadcasting errors in single_segment constraints.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

calculate_periods_per_period: docstring and falsy-last-period edge.

• Docstring still refers to timesteps; rename to periods.
• Using or treats 0 as falsy; if 0 is a valid last span, it will be overridden.

-    def calculate_periods_per_period(periods: pd.Index, periods_of_last_period: int | None = None) -> xr.DataArray:
-        """Calculate duration of each timestep as a 1D DataArray."""
+    def calculate_periods_per_period(periods: pd.Index, periods_of_last_period: int | None = None) -> xr.DataArray:
+        """Calculate length of each period; last value can be overridden."""
@@
-            np.append(periods_per_period, periods_of_last_period or periods_per_period[-1]),
+            np.append(periods_per_period, periods_per_period[-1] if periods_of_last_period is None else periods_of_last_period),

📝 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.

Suggested change

	def calculate_periods_per_period(periods: pd.Index, periods_of_last_period: int | None = None) -> xr.DataArray:
	"""Calculate duration of each timestep as a 1D DataArray."""
	years_per_year = np.diff(years)
	periods_per_period = np.diff(periods)
	return xr.DataArray(
	np.append(years_per_year, years_of_last_year or years_per_year[-1]),
	coords={'year': years},
	dims='year',
	name='years_per_year',
	np.append(periods_per_period, periods_of_last_period or periods_per_period[-1]),
	coords={'period': periods},
	dims='period',
	name='periods_per_period',
	)
	def calculate_periods_per_period(periods: pd.Index, periods_of_last_period: int | None = None) -> xr.DataArray:
	"""Calculate length of each period; last value can be overridden."""
	periods_per_period = np.diff(periods)
	return xr.DataArray(
	np.append(
	periods_per_period,
	periods_per_period[-1] if periods_of_last_period is None else periods_of_last_period,
	),
	coords={'period': periods},
	dims='period',
	name='periods_per_period',
	)

🤖 Prompt for AI Agents

In flixopt/flow_system.py around lines 179 to 187, the function docstring
incorrectly mentions "timesteps" and the logic for the fallback last period uses
`or` which treats 0 as falsy; change the docstring to say "periods" and replace
the fallback with an explicit None check (use periods_of_last_period if
periods_of_last_period is not None else periods_per_period[-1]) so that a valid
0 value is preserved when appending the last span.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Fix double time-summing and missing rename in total aggregation.

• temporal is summed over time twice (once unconditionally, then conditionally).
• When both parts exist, the returned DataArray keeps the periodic name, not '{element}->{effect}'.

Apply this diff:

-            temporal = temporal.sum('time')
-            if periodic.isnull().all():
+            # Reduce temporal along time once
+            temporal = temporal.sum('time')
+            if periodic.isnull().all():
                 return temporal.rename(f'{element}->{effect}')
-            if 'time' in temporal.indexes:
-                temporal = temporal.sum('time')
-            return periodic + temporal
+            total = periodic + temporal
+            return total.rename(f'{element}->{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.

Suggested change

	if periodic.isnull().all() and temporal.isnull().all():
	return xr.DataArray(np.nan)
	if temporal.isnull().all():
	return nontemporal.rename(f'{element}->{effect}')
	return periodic.rename(f'{element}->{effect}')
	temporal = temporal.sum('time')
	if nontemporal.isnull().all():
	if periodic.isnull().all():
	return temporal.rename(f'{element}->{effect}')
	if 'time' in temporal.indexes:
	temporal = temporal.sum('time')
	return nontemporal + temporal
	return periodic + temporal
	if periodic.isnull().all() and temporal.isnull().all():
	return xr.DataArray(np.nan)
	if temporal.isnull().all():
	return periodic.rename(f'{element}->{effect}')
	# Reduce temporal along time once
	temporal = temporal.sum('time')
	if periodic.isnull().all():
	return temporal.rename(f'{element}->{effect}')
	total = periodic + temporal
	return total.rename(f'{element}->{effect}')

🤖 Prompt for AI Agents

In flixopt/results.py around lines 579 to 589, temporal is being summed over
'time' twice and the final combined result isn't renamed; fix by removing the
unconditional temporal = temporal.sum('time') so temporal is summed only if it
contains the 'time' index, and when returning the combined periodic + temporal
DataArray, apply .rename(f'{element}->{effect}') so the total aggregation has
the correct name; keep the existing early-return renames for the single-part
cases and preserve the initial all-null check.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Weights semantics changed (scenario → period×scenario); clarify and test

Doc says “scenario weights”, but missing-weights path now creates a period×scenario grid and normalizes across all cells. This is a behavior change that can silently alter aggregation if consumers expect scenario-only weighting.

• Either update the docstring to reflect combined period×scenario weights and add tests, or
• Keep scenario-only defaults and handle period weighting where needed.

Optional diff to clarify intent via explicit dim-sum and docstring:

-    def weights(self) -> int | xr.DataArray:
-        """Returns the scenario weights of the FlowSystem. If None, return weights that are normalized to 1 (one)"""
+    def weights(self) -> int | xr.DataArray:
+        """Returns normalized weights aligned to model coords.
+        If self.flow_system.weights is None, create equal weights over ['period','scenario'] and normalize their sum to 1."""
         if self.flow_system.weights is not None:
             return self.flow_system.weights

-        weights = self.flow_system.fit_to_model_coords('weights', 1, dims=['period', 'scenario'])
-        return weights / weights.sum()
+        weights = self.flow_system.fit_to_model_coords('weights', 1, dims=['period', 'scenario'])
+        total = weights.sum(dim=list(weights.dims))
+        return xr.where(total != 0, weights / total, weights)

---

🏁 Script executed:

#!/bin/bash
# Find usages of model.weights and check expected dims/normalization.
rg -nP -C2 '\bweights\b' -g "flixopt/**" | sed -n '1,200p'

Length of output: 7553

---

Fix weights() docstring to reflect period×scenario grid flixopt/structure.py:171-178

• Docstring currently states “scenario weights” but this always returns a period×scenario DataArray normalized across both dims.
• Update the docstring and add/adjust tests for this behavior.
• [optional] Use explicit sum(dim=list(weights.dims)) and guard against divide-by-zero.

🤖 Prompt for AI Agents

In flixopt/structure.py around lines 171 to 178, the weights() docstring
incorrectly says "scenario weights" but the method returns a period×scenario
DataArray normalized across both dimensions; update the docstring to state it
returns period×scenario weights normalized to sum to 1 across all periods and
scenarios, and add/adjust tests to assert the shape (period, scenario) and that
weights.sum(dim=list(weights.dims)) == 1; when normalizing, compute total =
weights.sum(dim=list(weights.dims)) and guard against divide-by-zero by
returning zeros or raising a clear error if total == 0 before doing weights /
total.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Constraint contradicts bounds for periodic mode.

You set Effect1(periodic) with lower/upper 2.0–2.1 (Lines 121–123) but still assert a constraint enforcing Effect1(periodic) == 0. This will be infeasible. Drop or adapt the zero-equality for the bounds test.

Apply this diff:

-        assert_conequal(model.constraints['Effect1(periodic)'], model.variables['Effect1(periodic)'] == 0)
+        # No zero-equality here: periodic total is bounded [2.0, 2.1] above
+        assert 'Effect1(periodic)' in model.constraints

📝 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.

Suggested change

	assert_conequal(model.constraints['Effect1(periodic)'], model.variables['Effect1(periodic)'] == 0)
	assert_conequal(
	# No zero‐equality here: periodic total is bounded [2.0, 2.1] above
	assert 'Effect1(periodic)' in model.constraints
	assert_conequal(