fix: explicit adjacent-timestep alignment for linopy v1 compat#684
Open
FBumann wants to merge 2 commits into
Open
fix: explicit adjacent-timestep alignment for linopy v1 compat#684FBumann wants to merge 2 commits into
FBumann wants to merge 2 commits into
Conversation
flixopt builds adjacent-step constraints (storage energy balance, state transitions, consecutive-duration tracking, multi-period linking) by slicing the same variable at `[1:]` and `[:-1]` and combining them. The two slices carry different time/period labels by construction, and linopy's legacy semantics silently aligned them positionally. Under the new opt-in v1 semantics (`linopy.options['semantics']='v1'`), shared-dim labels must match exactly, so every such site raised. Relabel the lead slice onto the lag axis (or vice versa) via `.assign_coords(...)` so positional intent is explicit. For scalar `.isel(time=0)` / `.isel(time=-1)` patterns, pass `drop=True` to avoid leftover aux-coord conflicts. In `consecutive_duration_tracking`, the `lb` constraint must reference duration at the on-state moment, not the off-transition that follows — so the helper uses lag-label convention throughout. Verified with `linopy.options.set_value(semantics='v1')`: 383/386 `tests/test_math/` pass (the remaining 3 are pre-existing failures on main, unrelated to this change). No regression under legacy semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR refactors constraint construction across StorageModel, InvestmentModel, StatusModel, and duration/state-transition tracking functions to use explicit coordinate slicing with ChangesCoordinate alignment and dimensionality consistency
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/modeling.py (1)
867-874:⚠️ Potential issue | 🟠 Major | 💤 Low valueMissing linopy v1 label-alignment in
link_changes_to_level_with_binariestransition constraint
link_changes_to_level_with_binariesbuildslevel[t] = level[t-1] + increase[t] - decrease[t]with lead/lagiselslices but never relabels the lag slice onto the lead axis viaassign_coords(seeflixopt/modeling.py:868-873). This is inconsistent with nearby helpers that explicitly note linopy v1 requires matching labels on shared dims and fix it viastate.isel(lag).assign_coords(lead_coord)(and the same adjacent-slice pattern also exists incontinuous_transition_bounds).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@flixopt/modeling.py` around lines 867 - 874, The transition constraint in link_changes_to_level_with_binaries constructs level[t] == level[t-1] + increase[t] - decrease[t] using isel lead/lag slices but does not align the lag slice labels to the lead axis; update the constraint to reassign coordinates on the lag slices (e.g., use level_variable.isel({coord: slice(None, -1)}).assign_coords({coord: level_variable.isel({coord: slice(1, None)}).coords[coord]}) or the equivalent pattern used elsewhere before adding the constraint so that level_variable, increase_variable, and decrease_variable share the same coord labels (follow the assign_coords(lag->lead) pattern used in other helpers like continuous_transition_bounds) to satisfy linopy v1 label alignment requirements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@flixopt/modeling.py`:
- Around line 729-732: The constraint call creating transition via
model.add_constraints is misformatted; reformat the call to match the forward
constraint style by placing the entire expression as the first argument on its
own line and the name= keyword on a separate line (i.e., break after
model.add_constraints(, put the expression involving activate.isel(lead) -
deactivate.isel(lead) == state.isel(lead) -
state.isel(lag).assign_coords(lead_coord) on its own line, then the
name=f'{name}|transition' on the next line) so the parentheses and arguments
align consistently with the other add_constraints usages (symbols: transition,
model.add_constraints, activate, deactivate, state, isel, assign_coords, name).
- Around line 415-418: The constraint expression assigned to
constraints['forward'] is split across too many lines and fails ruff formatting;
reformat the call to model.add_constraints so the whole boolean expression and
its arguments fit on fewer lines (e.g., place the left-hand expression, the
comparison and the name argument on the same or fewer lines) while keeping the
same semantics: update the code around constraints['forward'] where
model.add_constraints is called with
duration.isel(lead).assign_coords(lag_coord) <= duration.isel(lag) +
duration_per_step.isel(lag) and name=f'{duration.name}|forward' so it conforms
to ruff/PEP8 line-length/formatting rules.
---
Outside diff comments:
In `@flixopt/modeling.py`:
- Around line 867-874: The transition constraint in
link_changes_to_level_with_binaries constructs level[t] == level[t-1] +
increase[t] - decrease[t] using isel lead/lag slices but does not align the lag
slice labels to the lead axis; update the constraint to reassign coordinates on
the lag slices (e.g., use level_variable.isel({coord: slice(None,
-1)}).assign_coords({coord: level_variable.isel({coord: slice(1,
None)}).coords[coord]}) or the equivalent pattern used elsewhere before adding
the constraint so that level_variable, increase_variable, and decrease_variable
share the same coord labels (follow the assign_coords(lag->lead) pattern used in
other helpers like continuous_transition_bounds) to satisfy linopy v1 label
alignment requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4b37b0b-0c03-4311-a1f4-a25fab04ecdc
📒 Files selected for processing (3)
flixopt/components.pyflixopt/features.pyflixopt/modeling.py
Apply the same lag→lead `.assign_coords(...)` and `drop=True` pattern to two more helpers that build adjacent-step constraints with positional `.isel()` — currently unused by flixopt's tests but defined in the public modeling primitives, so they would raise under linopy v1 when called. Also collapses ruff-formatted lines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FBumann
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/modeling.py (1)
389-390:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix scalar handling for
duration_per_step(runtime breakage) + checkmax_changeinitial bound indexing
consecutive_duration_tracking(..., duration_per_step: int | float | xr.DataArray)declares scalars are allowed, butflixopt/modeling.pyunconditionally callsduration_per_step.sum(...)andduration_per_step.isel(...)(includingduration_per_step.isel({duration_dim: 0}, drop=True)), so passing a plainint/floatwill raise at runtime.Also re-check
BoundingPatterns.continuous_transition_bounds: whileactivate/deactivatefor the initial step are sliced tot=0,initial_bound = max_change * (activate.isel({coord: 0}, drop=True) + deactivate.isel({coord: 0}, drop=True))leavesmax_changeunsliced. Ifmax_changeis time-indexedxr.DataArray(not scalar), this can broadcast it across time and overconstrain/mismatch the initial-step constraint.💡 Minimal fix to honor the declared scalar/DataArray contract
- mega = duration_per_step.sum(duration_dim) + (previous_duration if previous_duration is not None else 0) + mega = _scalar_safe_reduce(duration_per_step, duration_dim, method='sum') + ( + previous_duration if previous_duration is not None else 0 + ) @@ + duration_step_lag = _scalar_safe_isel(duration_per_step, lag) constraints['forward'] = model.add_constraints( - duration.isel(lead).assign_coords(lag_coord) <= duration.isel(lag) + duration_per_step.isel(lag), + duration.isel(lead).assign_coords(lag_coord) <= duration.isel(lag) + duration_step_lag, name=f'{duration.name}|forward', ) @@ - == (duration_per_step.isel({duration_dim: 0}, drop=True) + previous_duration) + == (_scalar_safe_isel_drop(duration_per_step, duration_dim, 0) + previous_duration) * state.isel({duration_dim: 0}, drop=True),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@flixopt/modeling.py` around lines 389 - 390, The code assumes duration_per_step is always an xr.DataArray but the signature allows int/float; update consecutive_duration_tracking to handle scalar vs DataArray: if duration_per_step is a plain int/float compute mega = float(duration_per_step) * number_of_steps + (previous_duration or 0) or convert scalar to an xr.DataArray matching the expected dims before calling .sum(...) and .isel(...); only call .sum and .isel when duration_per_step is an xr.DataArray. Also fix BoundingPatterns.continuous_transition_bounds: when computing initial_bound = max_change * (activate.isel({coord: 0}, drop=True) + deactivate.isel({coord: 0}, drop=True)), ensure max_change is sliced the same way (e.g., use max_change.isel({coord: 0}, drop=True) when max_change is a DataArray, otherwise use the scalar directly) so a time-indexed max_change is not left unsliced and broadcasting/mismatches are avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@flixopt/modeling.py`:
- Around line 389-390: The code assumes duration_per_step is always an
xr.DataArray but the signature allows int/float; update
consecutive_duration_tracking to handle scalar vs DataArray: if
duration_per_step is a plain int/float compute mega = float(duration_per_step) *
number_of_steps + (previous_duration or 0) or convert scalar to an xr.DataArray
matching the expected dims before calling .sum(...) and .isel(...); only call
.sum and .isel when duration_per_step is an xr.DataArray. Also fix
BoundingPatterns.continuous_transition_bounds: when computing initial_bound =
max_change * (activate.isel({coord: 0}, drop=True) + deactivate.isel({coord: 0},
drop=True)), ensure max_change is sliced the same way (e.g., use
max_change.isel({coord: 0}, drop=True) when max_change is a DataArray, otherwise
use the scalar directly) so a time-indexed max_change is not left unsliced and
broadcasting/mismatches are avoided.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.assign_coords(...)at the handful of sites where flixopt combines[1:]and[:-1]of the same variable to build difference/transition constraints. Under linopy's legacy semantics these aligned silently by position; the upcoming v1 convention (PyPSA/linopy#717) requires shared-dim labels to match exactly.drop=Trueto scalar.isel(time=0)/.isel(time=-1)patterns so leftover aux coords don't conflict on combination.consecutive_duration_tracking, switch to a lag-label convention throughout so thelbconstraint references duration at the on-state moment (the off-transition timestep hasduration=0).Sites
flixopt/components.py:_build_energy_balance_lhs— storage energy balance.flixopt/components.py:_add_cluster_cyclic_constraint,_add_initial_final_constraints— scalar.iselcalls.flixopt/modeling.py:ModelingPrimitives.consecutive_duration_tracking— forward / backward / initial / lb.flixopt/modeling.py:BoundingPatterns.state_transition_bounds— transition + initial.flixopt/features.py:InvestmentModellinked-periods constraint.flixopt/features.py:StatusModel._add_cluster_cyclic_constraint.Validation
With
linopy.options.set_value(semantics='v1'):tests/test_math/: 383 pass / 3 fail (was 236/150). The 3 remaining failures are pre-existing onmain(TestClusteringExact::test_storage_cyclic_charge_discharge_pattern), unrelated to this change.tests/(excludingtest_math/): no v1-specific regressions — same 68 pre-existing failures under both legacy and v1.Test plan
pytest tests/test_math/passes under legacy semantics (no behavior change).pytest tests/test_math/passes under v1 (linopy.options.set_value(semantics='v1')) with only the 3 pre-existing failures.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor