Feat/add piecewise linear clean+linear #10
Conversation
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
…_SUFFIX = "_fill". linopy/model.py — - Added method: str = "sos2" parameter to add_piecewise_constraints() - Updated docstring with the new parameter and incremental formulation notes - Refactored: extracted _add_pwl_sos2() (existing SOS2 logic) and added _add_pwl_incremental() (new delta formulation) - Added _check_strict_monotonicity() static method - method="auto" checks monotonicity and picks accordingly - Numeric coordinate validation only enforced for SOS2 test/test_piecewise_constraints.py — Added TestIncrementalFormulation (10 tests) covering: single variable, two breakpoints, dict case, non-monotonic error, decreasing monotonic, auto-select incremental/sos2, invalid method, extra coordinates. Added TestIncrementalSolverIntegration (Gurobi-gated).
…f(dim).rename() 2. Filling-order constraints: replaced per-segment individual add_constraints calls with a single vectorized constraint via xr.concat + LinearExpression 3. Mask computation: replaced loop over segments with vectorized slice + rename 4. Coordinate lists: unified extra_coords/lambda_coords — lambda_coords = extra_coords + [bp_dim_index], eliminating duplicate list comprehensions
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThe pull request introduces a new piecewise-linear constraint formulation called "incremental" alongside the existing "sos2" method. It adds suffix constants, extends the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
7 tasks
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@linopy/model.py`:
- Around line 934-941: The _check_strict_monotonicity function currently
evaluates monotonicity over the entire DataArray at once and fails when
different variables have opposite directions or when diffs contain NaNs; change
it to compute diffs = breakpoints.diff(dim) and then iterate (or apply a
vectorized reduction) over all orthogonal slices along dim (e.g., per-variable
slices) ignoring NaNs in each slice, and for each slice consider it strictly
monotonic if all non-NaN diffs are either >0 or <0; return True only if every
slice meets that condition. Ensure you reference _check_strict_monotonicity, the
breakpoints DataArray and dim when locating and updating the logic.
🧹 Nitpick comments (1)
linopy/constants.py (1)
39-44: Add type annotations for the new PWL suffix constants.These new constants are untyped; annotating them as
strkeeps typing consistent and helps mypy.♻️ Suggested change
-PWL_DELTA_SUFFIX = "_delta" -PWL_FILL_SUFFIX = "_fill" +PWL_DELTA_SUFFIX: str = "_delta" +PWL_FILL_SUFFIX: str = "_fill"As per coding guidelines: Use type hints and mypy for type checking in Python files.
linopy/model.py
Outdated
Comment on lines
934
to
941
| @staticmethod | ||
| def _check_strict_monotonicity(breakpoints: DataArray, dim: str) -> bool: | ||
| """Check if breakpoints are strictly monotonic along dim.""" | ||
| diffs = breakpoints.diff(dim) | ||
| # All diffs must be either all positive or all negative (strictly monotonic) | ||
| all_positive = bool((diffs > 0).all()) | ||
| all_negative = bool((diffs < 0).all()) | ||
| return all_positive or all_negative |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the file
fd -t f "model.py" linopy/Repository: FBumann/linopy
Length of output: 73
🏁 Script executed:
# Get the specific lines mentioned in the review
cat -n linopy/model.py | sed -n '930,945p'Repository: FBumann/linopy
Length of output: 946
🏁 Script executed:
# Search for context around this function and its usage
rg "_check_strict_monotonicity" -B 5 -A 15Repository: FBumann/linopy
Length of output: 3546
🏁 Script executed:
# Check if there are any tests for this function
rg "_check_strict_monotonicity" --type py -lRepository: FBumann/linopy
Length of output: 73
🏁 Script executed:
# Get more context around the function calls
cat -n linopy/model.py | sed -n '750,780p'Repository: FBumann/linopy
Length of output: 1423
🏁 Script executed:
# Check the full _add_pwl_link function to understand the context better
cat -n linopy/model.py | sed -n '750,820p'Repository: FBumann/linopy
Length of output: 3246
🏁 Script executed:
# Look for tests related to PWL and monotonicity
fd -t f "test.*\.py" | xargs rg "pwl|monotonic" -lRepository: FBumann/linopy
Length of output: 92
🏁 Script executed:
# Check imports to understand what DataArray is
head -50 linopy/model.pyRepository: FBumann/linopy
Length of output: 1157
🏁 Script executed:
# Look at the PWL tests to understand the use cases
cat test/test_piecewise_constraints.py | head -100Repository: FBumann/linopy
Length of output: 3313
🏁 Script executed:
# Search for tests specifically about monotonicity
rg "monotonic|opposite" test/test_piecewise_constraints.py -B 2 -A 5Repository: FBumann/linopy
Length of output: 1619
🏁 Script executed:
# Check if there are any tests with NaNs in breakpoints
rg "nan|NaN" test/test_piecewise_constraints.py -B 2 -A 5Repository: FBumann/linopy
Length of output: 1424
🏁 Script executed:
# Look at the full _add_pwl_link signature to understand the dict case
rg "def _add_pwl_link" -A 30 linopy/model.pyRepository: FBumann/linopy
Length of output: 40
🏁 Script executed:
# Get the full signature of _add_pwl_link
cat -n linopy/model.py | sed -n '665,680p'Repository: FBumann/linopy
Length of output: 876
🏁 Script executed:
# Look for dict test with opposite monotonicity
rg "opposite|decreasing.*increasing|increasing.*decreasing" test/test_piecewise_constraints.py -B 5 -A 10Repository: FBumann/linopy
Length of output: 40
🏁 Script executed:
# Check if there's an existing test for dict with different monotonicities
rg "dict_of_variables" -A 20 test/test_piecewise_constraints.py | head -60Repository: FBumann/linopy
Length of output: 801
🏁 Script executed:
# Look at tests showing what happens with dict and non-monotonic
cat test/test_piecewise_constraints.py | sed -n '100,250p'Repository: FBumann/linopy
Length of output: 5197
🏁 Script executed:
# Look for any tests that show opposite monotonic directions
cat test/test_piecewise_constraints.py | sed -n '60,130p'Repository: FBumann/linopy
Length of output: 2503
🏁 Script executed:
# Verify the xarray API for diff, notnull, and the proposed operations
python3 << 'EOF'
import xarray as xr
import numpy as np
# Simulate the proposed fix logic
breakpoints = xr.DataArray(
[[0, 50, 100], [100, 50, 0]], # power increasing, efficiency decreasing
dims=["var", "bp"],
coords={"var": ["power", "efficiency"], "bp": [0, 1, 2]},
)
print("Breakpoints shape:", breakpoints.shape)
print("Breakpoints:\n", breakpoints.values)
diffs = breakpoints.diff("bp")
print("\nDiffs shape:", diffs.shape)
print("Diffs:\n", diffs.values)
valid = diffs.notnull()
print("\nValid shape:", valid.shape)
print("Valid:\n", valid.values)
all_positive = ((diffs > 0) | ~valid).all(dim="bp")
print("\nAll positive (shape", all_positive.shape, "):\n", all_positive.values)
all_negative = ((diffs < 0) | ~valid).all(dim="bp")
print("\nAll negative (shape", all_negative.shape, "):\n", all_negative.values)
result = bool((all_positive | all_negative).all())
print("\nFinal result (opposite directions):", result)
# Now test with NaNs
breakpoints_nan = xr.DataArray(
[[0, 50, np.nan, 100], [100, 50, 30, np.nan]],
dims=["var", "bp"],
coords={"var": ["power", "efficiency"], "bp": [0, 1, 2, 3]},
)
print("\n\n=== Test with NaNs ===")
print("Breakpoints shape:", breakpoints_nan.shape)
print("Breakpoints:\n", breakpoints_nan.values)
diffs_nan = breakpoints_nan.diff("bp")
print("\nDiffs shape:", diffs_nan.shape)
print("Diffs:\n", diffs_nan.values)
valid_nan = diffs_nan.notnull()
all_positive_nan = ((diffs_nan > 0) | ~valid_nan).all(dim="bp")
all_negative_nan = ((diffs_nan < 0) | ~valid_nan).all(dim="bp")
print("\nAll positive:\n", all_positive_nan.values)
print("All negative:\n", all_negative_nan.values)
result_nan = bool((all_positive_nan | all_negative_nan).all())
print("\nFinal result (with NaNs):", result_nan)
EOFRepository: FBumann/linopy
Length of output: 176
🏁 Script executed:
# Check the current behavior with opposite directions to confirm the issue
python3 << 'EOF'
import xarray as xr
import numpy as np
# Current implementation test
breakpoints = xr.DataArray(
[[0, 50, 100], [100, 50, 0]], # power increasing, efficiency decreasing
dims=["var", "bp"],
coords={"var": ["power", "efficiency"], "bp": [0, 1, 2]},
)
diffs = breakpoints.diff("bp")
print("Diffs:\n", diffs.values)
# Current implementation
all_positive = bool((diffs > 0).all())
all_negative = bool((diffs < 0).all())
current_result = all_positive or all_negative
print(f"\nCurrent implementation: all_positive={all_positive}, all_negative={all_negative}")
print(f"Current result (would be used for incremental): {current_result}")
print("This shows the issue: opposite directions are rejected!")
EOFRepository: FBumann/linopy
Length of output: 176
🏁 Script executed:
# Install xarray and numpy first
pip install xarray numpy -q && python3 << 'EOF'
import xarray as xr
import numpy as np
# Current implementation test
breakpoints = xr.DataArray(
[[0, 50, 100], [100, 50, 0]], # power increasing, efficiency decreasing
dims=["var", "bp"],
coords={"var": ["power", "efficiency"], "bp": [0, 1, 2]},
)
diffs = breakpoints.diff("bp")
print("Diffs:\n", diffs.values)
# Current implementation
all_positive = bool((diffs > 0).all())
all_negative = bool((diffs < 0).all())
current_result = all_positive or all_negative
print(f"\nCurrent implementation: all_positive={all_positive}, all_negative={all_negative}")
print(f"Current result (rejects opposite directions): {current_result}")
# Proposed implementation
valid = diffs.notnull()
all_positive_proposed = ((diffs > 0) | ~valid).all(dim="bp")
all_negative_proposed = ((diffs < 0) | ~valid).all(dim="bp")
proposed_result = bool((all_positive_proposed | all_negative_proposed).all())
print(f"\nProposed implementation:")
print(f" all_positive per row: {all_positive_proposed.values}")
print(f" all_negative per row: {all_negative_proposed.values}")
print(f" final result (accepts opposite directions): {proposed_result}")
EOFRepository: FBumann/linopy
Length of output: 602
🏁 Script executed:
# Test with NaNs to verify the proposed fix handles them correctly
pip install xarray numpy -q && python3 << 'EOF'
import xarray as xr
import numpy as np
# Test with NaNs
breakpoints_nan = xr.DataArray(
[[0, 50, np.nan, 100], [100, 50, 30, np.nan]],
dims=["var", "bp"],
coords={"var": ["power", "efficiency"], "bp": [0, 1, 2, 3]},
)
print("Breakpoints with NaNs:")
print(breakpoints_nan.values)
diffs_nan = breakpoints_nan.diff("bp")
print("\nDiffs with NaNs:")
print(diffs_nan.values)
# Current implementation - NaNs break it
all_positive_curr = bool((diffs_nan > 0).all())
all_negative_curr = bool((diffs_nan < 0).all())
print(f"\nCurrent: all_positive={all_positive_curr}, all_negative={all_negative_curr}, result={all_positive_curr or all_negative_curr}")
# Proposed implementation
valid = diffs_nan.notnull()
all_positive_prop = ((diffs_nan > 0) | ~valid).all(dim="bp")
all_negative_prop = ((diffs_nan < 0) | ~valid).all(dim="bp")
proposed_result = bool((all_positive_prop | all_negative_prop).all())
print(f"\nProposed:")
print(f" valid mask per row: {valid.values}")
print(f" all_positive per row: {all_positive_prop.values}")
print(f" all_negative per row: {all_negative_prop.values}")
print(f" final result (handles NaNs): {proposed_result}")
EOFRepository: FBumann/linopy
Length of output: 678
🏁 Script executed:
# Look for any test that shows the dict case with opposite monotonicity issue
cat test/test_piecewise_constraints.py | grep -A 40 "test_dict_of_variables"Repository: FBumann/linopy
Length of output: 1574
Fix monotonicity check to support per-variable directions and handle masked NaNs.
The current implementation incorrectly rejects valid cases where:
- Different variables in a dict have opposite monotonic directions (e.g., power increasing while efficiency decreases)
- NaN values used for masking are present in the diffs
The check evaluates monotonicity globally across the full DataArray, but should evaluate per slice along dim to allow independent monotonic directions. NaN diffs should be ignored.
🛠️ Suggested fix
- diffs = breakpoints.diff(dim)
- # All diffs must be either all positive or all negative (strictly monotonic)
- all_positive = bool((diffs > 0).all())
- all_negative = bool((diffs < 0).all())
- return all_positive or all_negative
+ diffs = breakpoints.diff(dim)
+ valid = diffs.notnull()
+ # Each slice must be strictly increasing or strictly decreasing; ignore NaNs
+ all_positive = ((diffs > 0) | ~valid).all(dim=dim)
+ all_negative = ((diffs < 0) | ~valid).all(dim=dim)
+ return bool((all_positive | all_negative).all())🤖 Prompt for AI Agents
In `@linopy/model.py` around lines 934 - 941, The _check_strict_monotonicity
function currently evaluates monotonicity over the entire DataArray at once and
fails when different variables have opposite directions or when diffs contain
NaNs; change it to compute diffs = breakpoints.diff(dim) and then iterate (or
apply a vectorized reduction) over all orthogonal slices along dim (e.g.,
per-variable slices) ignoring NaNs in each slice, and for each slice consider it
strictly monotonic if all non-NaN diffs are either >0 or <0; return True only if
every slice meets that condition. Ensure you reference
_check_strict_monotonicity, the breakpoints DataArray and dim when locating and
updating the logic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes # (if applicable).
Changes proposed in this Pull Request
Checklist
doc.doc/release_notes.rstof the upcoming release is included.Summary by CodeRabbit
Release Notes
methodparameter: "sos2" (default), "incremental", or "auto".✏️ Tip: You can customize this high-level summary in your review settings.