Add combination tests for piecewise, status, investment & effects interactions

[FBumann]

Summary

• Add test_combinations.py with 20 tests (60 runs with the 3-mode optimize fixture) covering feature interactions that were previously untested
• Each test is sensitivity-verified: it produces a different result if the feature it tests were removed

Test Coverage

Class	Tests	Covers
TestPiecewiseWithInvestment	2	Piecewise conversion + invest sizing; piecewise invest cost + optional skip
TestPiecewiseWithStatus	4	Non-1:1 piecewise + startup cost; gap-based min load; no zero point + status off; no zero point + startup cost
TestPiecewiseThreeSegments	3	3-segment piecewise at high/low/mid load
TestStatusWithEffects	2	Startup cost on CO2 effect; active-hour cost on multiple effects
TestInvestWithRelativeMinimum	1	Invest sizing with relative_minimum forcing off-state
TestConversionWithTimeVaryingEffects	2	Time-varying effects_per_flow_hour; dual-output CHP with time-varying costs
TestPiecewiseInvestWithStatus	1	Piecewise invest (economy of scale) + startup costs
TestStatusWithMultipleConstraints	2	startup_limit + max_downtime; min_uptime + min_downtime
TestEffectsWithConversion	2	Effect share_from_temporal + investment; effect maximum_total + status
TestInvestWithEffects	1	invest_per_size on non-cost (CO2) effect with maximum_total

Key scenarios tested

• Piecewise conversion without zero point (no off-state piece) + status interaction
• 3+ segment piecewise linearization
• Piecewise investment with economy of scale in segment 2
• Status effects (startup/active-hour) on non-cost effects (CO2)
• Investment sizing constrained by relative_minimum
• Time-varying effects through linear converters

Test plan

• All 60 test runs pass on main (pytest tests/test_math/test_combinations.py -v)
• All existing tests unaffected (pytest tests/test_math/ -v)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Added extensive integration test suite covering complex feature interactions, constraints, time-varying effects, and edge cases to improve system reliability.

[coderabbitai]

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Adds a large new test module introducing comprehensive integration tests for piecewise conversions, investments, status/startup constraints, time-varying effects, multi-output conversions, and associated observables across many scenarios (≈1,236 added lines).

Changes

Cohort / File(s)

Summary

New Test Suite tests/test_math/test_combinations.py

Adds a comprehensive test file (~+1236 lines) covering: piecewise conversions with investments, multi-segment piecewise behavior, status/startup/min up/min down interactions, relative minimums, time-varying and per-hour effects, multi-output conversions (CHP, boilers, optional skips), observables (cost, CO₂, flows, statuses, invested sizes), and numerous edge-case scenarios organized into multiple test classes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• Add combination tests for piecewise, status, investment & effects interactions #608: Adds/updates the same tests/test_math/test_combinations.py test suite with extensive combination tests covering piecewise conversions, investments, statuses, and effects — likely the same or a closely related change.

Poem

🐰 I hop through tests in morning light,
Piecewise puzzles, startups, all delight,
Investments sprout and flows entwine,
I nibble bugs and mark the line,
A joyful rabbit in test-time flight.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into main
Title check	✅ Passed	The title clearly summarizes the main change: adding comprehensive combination tests for piecewise conversions, status, investment, and effects interactions.
Description check	✅ Passed	The description is thorough and well-structured, with test coverage details, scenario descriptions, and test plan confirmation, exceeding the template requirements.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch tests/combinations

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@tests/test_math/test_combinations.py`:
- Around line 986-1014: The docstring text "Fuel emits 0.5 kg CO2/kWh" is
inconsistent with the test setup where the gas flow sets
effects_per_flow_hour={'costs': 1, 'CO2': 0.1}; update the docstring to state
"Fuel emits 0.1 kg CO2/kWh" (or change the Flow's 'CO2' value to 0.5 if you
prefer to keep the original docstring) so the comment matches the actual values
used in fx.Effect('CO2', ...) and the fx.Source('GasSrc') / fx.Flow(...)
effects_per_flow_hour configuration.

🧹 Nitpick comments (2)

tests/test_math/test_combinations.py (2)

264-277: Status validation for min_uptime is correct but subtly incomplete at the trailing on-block.

The loop at lines 266–277 checks that every "on" timestep has at least one "on" neighbor. However, if the entire trailing block is exactly 1 timestep long and is not the last index, the check catches it. If it is the last index, the i == len(status) - 1 guard auto-passes — which is the intended boundary relaxation.

This works but is somewhat fragile: it doesn't directly verify "on-block length ≥ 2" (the actual min_uptime semantics). The later test test_min_uptime_with_min_downtime (line 896) uses an explicit block-length counter, which is more robust. Consider using the same pattern here for consistency.

---

896-920: Block-length validation loops don't check the final block at end-of-horizon.

The on-block loop (lines 896–905) and the off-block loop (lines 908–920) only assert block lengths when a transition occurs within the horizon. A trailing block that reaches the last timestep is never validated. This is intentionally correct for boundary-relaxed constraints, but adding a brief comment explaining why the trailing block is deliberately unchecked would improve readability.

📝 Suggested comment addition

         for i, s in enumerate(status):
             if s > 0.5:
                 on_block_len += 1
             else:
                 if on_block_len > 0:
                     assert on_block_len >= 2, (
                         f'min_uptime violated: on-block of {on_block_len} at t<{i}: status={status}'
                     )
                 on_block_len = 0
+        # Trailing on-block at end of horizon is not checked: min_uptime constraint is relaxed at boundary

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@tests/test_math/test_combinations.py`:
- Around line 1032-1042: The min_uptime check currently only asserts inside the
loop when an off-state is encountered, so a trailing on-block isn't validated;
after the loop that iterates over status and updates on_block_len, add a final
check that if on_block_len > 0 then assert on_block_len >= 2 (or use the
configured min_uptime) and include the same failure message format referencing
status and the final index, ensuring the variables used (status, on_block_len,
min_uptime) match the existing names in the test.

🧹 Nitpick comments (3)

tests/test_math/test_combinations.py (3)

960-975: Startup counting logic assumes previous_flow_rate > 0 implies initial ON state.

Line 970 counts any status[0] > 0.5 as a startup (since i == 0 satisfies the or condition), then Line 973–974 subtracts it if the boiler was already on. This works but is fragile — if previous_flow_rate were changed to 0 in a future refactor, the subtraction would incorrectly reduce the count. Consider making the initial-state logic explicit:

Suggested simplification

-        startups = sum(1 for i in range(len(status)) if status[i] > 0.5 and (i == 0 or status[i - 1] < 0.5))
-        # Account for carry-over: was on before, so first on isn't a startup
-        # if status[0] > 0.5 then it was already on (previous_flow_rate=10)
-        if status[0] > 0.5:
-            startups -= 1  # Not a startup, was already on
-        assert startups <= 2, f'startup_limit violated: {startups} startups, status={status}'
+        # Count startups: transition from off→on. At t=0, previous was on (previous_flow_rate=10).
+        prev_on = True  # previous_flow_rate=10 means was ON
+        startups = sum(
+            1 for i in range(len(status))
+            if status[i] > 0.5 and not (prev_on if i == 0 else status[i - 1] > 0.5)
+        )
+        assert startups <= 2, f'startup_limit violated: {startups} startups, status={status}'

---

596-609: test_startup_cost_on_co2_effect: assertions are loose — consider tighter validation.

The test only checks CO2 ≤ 60 and startups ≤ 1, without asserting an exact cost. This means a regression that changes cost behavior (but still satisfies the constraints) would go undetected. Consider adding an assert_allclose on costs for a specific expected value, similar to how other tests in this file assert exact objectives.

---

1059-1062: Loose cost bounds in test_min_uptime_with_min_downtime.

The assertion 120 < costs < 240 is a very wide range. Given min_uptime=2 and min_downtime=2 with 6 timesteps, the optimal pattern is deterministic (e.g., ON for 4 hours, OFF for 2, or ON 2, OFF 2, ON 2). You could compute the exact expected cost for the optimal block pattern and use assert_allclose, which would catch more subtle regressions.