Skip to content

fix: include Investment costs in compute_effect_contributions#135

Merged
FBumann merged 1 commit intomainfrom
fix/investment-contributions
Apr 19, 2026
Merged

fix: include Investment costs in compute_effect_contributions#135
FBumann merged 1 commit intomainfrom
fix/investment-contributions

Conversation

@FBumann
Copy link
Copy Markdown
Owner

@FBumann FBumann commented Apr 19, 2026

Summary

compute_effect_contributions previously only attributed Sizing costs to contributors; Investment costs were missing, causing the validation check to fail (ValueError: Effect contributions do not sum to solver totals) for any model using Investment.

This PR adds _compute_investment_lump() which attributes the four Investment cost types to the originating flow:

  • effects_per_size_at_build × invest--size_at_build (CAPEX scaled by built capacity)
  • effects_fixed_at_build × invest--build (CAPEX fixed when built)
  • effects_per_size_recurring × flow--size (recurring O&M scaled by capacity)
  • effects_fixed_recurring × invest--active (recurring fixed O&M)

Mirrors the model's own lump_direct accumulation in _create_effects().

Closes #84.

Test plan

  • 7 xfail markers removed from test_multi_period.py validate-pipeline tests that now pass
  • 1 xfail remains: period-varying contribution_from Leontief inverse — separate bug tracked in fix: effect_contributions Leontief inverse fails for period-varying contribution_from #134
  • Full suite: 389 passed, 1 xfailed (was 382 passed, 8 xfailed)
  • Validated result.stats.effect_contributions['total'].sum('contributor') == result.effect_totals for multi-period Investment

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Fixed cost contribution calculations for investment and sizing components in multi-period analyses.
  • Tests

    • Investment-related test cases for period-varying scenarios now pass correctly.

compute_effect_contributions previously only attributed Sizing costs to
contributors; Investment costs were missing, causing the validation
check to fail for any model using Investment.

Add _compute_investment_lump() to attribute the four Investment cost
types (per_size_at_build, fixed_at_build, per_size_recurring,
fixed_recurring) to the originating flow, matching the model's
own lump_direct accumulation.

Removes 7 xfail markers from validate-pipeline tests that now pass.
One remaining xfail (period-varying contribution_from in Leontief)
is a separate bug tracked in #134.

Closes #84.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 19, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

The pull request refactors flow contribution computation in contributions.py by renaming the periodic helper to _compute_sizing_lump, adding a new _compute_investment_lump() function to compute investment-related contributions, and updating compute_effect_contributions to combine sizing and investment lumps. Related tests are updated to remove xfail markers.

Changes

Cohort / File(s) Summary
Contribution computation refactoring
src/fluxopt/contributions.py
Renamed _compute_periodic to _compute_sizing_lump for flow/storage sizing contributions. Added _compute_investment_lump() to sum investment solver variables (capex per-size, capex fixed, and periodic variants), align by invest_flow, and reindex. Updated compute_effect_contributions to compute total flow lump as sizing + investment, and added FlowsData to TYPE_CHECKING imports.
Test marker updates
tests/math_port/test_multi_period.py
Updated _XFAIL_REASON to reflect new expected-failure cause (Leontief inverse for period-varying contributions #134 instead of investment support #84). Removed _xfail_if_validate(optimize) calls from four investment-related test cases, allowing them to run in the optimize→save→reload→validate pipeline without xfail status.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

  • PR #85 — Introduces investment variables, data structures, and per-investment effects that this PR complements by enabling proper contribution tracking for investment costs.

Poem

🐰 The lump now splits in two, we see,
Sizing and investment dance with glee!
No more periodic mystery,
Contributions bloom perfectly! 🌿

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically identifies the main change: adding Investment costs support to compute_effect_contributions.
Description check ✅ Passed The description provides comprehensive context covering the problem, solution, implementation details of the four investment cost types, and test validation results.
Linked Issues check ✅ Passed The PR successfully addresses issue #84's primary requirement to include Investment costs in compute_effect_contributions through the new _compute_investment_lump() helper that attributes four investment cost types to flows, and removes 7 xfail markers from passing tests.
Out of Scope Changes check ✅ Passed All changes are directly scoped to addressing issue #84: the new _compute_investment_lump() helper, renaming _compute_periodic to _compute_sizing_lump for clarity, updating compute_effect_contributions to combine sizing and investment lumps, and removing passing xfail markers.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/investment-contributions

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

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

@FBumann FBumann enabled auto-merge (squash) April 19, 2026 15:23
@FBumann FBumann disabled auto-merge April 19, 2026 15:24
@FBumann FBumann merged commit 6bc2d4b into main Apr 19, 2026
9 of 10 checks passed
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/fluxopt/contributions.py 96.42% <100.00%> (+0.71%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@FBumann FBumann added type:fix Something isn't working area:effects Effect system, objectives, contributions area:sizing Sizing / Investment, capacity optimization labels Apr 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:effects Effect system, objectives, contributions area:sizing Sizing / Investment, capacity optimization type:fix Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: effect_contributions does not support investment costs or period decomposition

1 participant