refac: rename cross-effect API and add once domain#106
Merged
FBumann merged 3 commits intofeat/component-status-piecewisefrom Mar 16, 2026
Merged
refac: rename cross-effect API and add once domain#106FBumann merged 3 commits intofeat/component-status-piecewisefrom
FBumann merged 3 commits intofeat/component-status-piecewisefrom
Conversation
|
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCross-effect API renamed from Changes
Sequence DiagramsequenceDiagram
participant Build as EffectsData.build()
participant Matrix as cf_once Matrix
participant Model as Model._create_effects()
participant Leontief as Leontief Inverse
participant Constraint as effect_once Constraint
Build->>Build: Extract cross_once from Effect fields
Build->>Matrix: Construct once_mat from cross_once
Build->>Build: Initialize cf_once with once_mat if non-zero
Model->>Model: Compute once_direct from effect definition
Model->>Leontief: Apply Leontief(cf_once) to propagate cross-effects
Leontief->>Model: Return cross-term contribution
Model->>Model: once_rhs = cross + once_direct
Model->>Constraint: Add constraint: effect_once == once_rhs
Constraint->>Constraint: Enforce once-domain with cross-effect propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 unit tests (beta)
📝 Coding Plan
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 |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
tests/math/test_contributions.py (1)
143-143: Consider adding test coverage forcross_once.This file thoroughly tests
cross_periodicandcross_temporal, but the newcross_oncefield (one-time investment cross-effects) doesn't have dedicated tests here. Given that the PR addscf_onceLeontief application in bothmodel.pyandcontributions.py, a test similar totest_sizing_cross_effect_investmentbut usingcross_oncewould validate the new functionality.Would you like me to draft a test case for
cross_oncecovering one-time investment cross-effects (e.g., embodied emissions in CAPEX)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/math/test_contributions.py` at line 143, Add a new unit test exercising the new cross_once behavior: mirror the existing test_sizing_cross_effect_investment but use Effect(..., cross_once={'co2': <value>}) and ensure the contributions pipeline applies cf_once (the Leontief one-time cross-effect) from model.py/contributions.py, asserting the one-time embodied emissions are added to CAPEX/sizing contributions as expected; reference the Effect creation with cross_once, the cf_once application path in contributions.py, and the model's handling of cf_once to locate where to validate the result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fluxopt/contributions.py`:
- Around line 404-407: The module docstring claims "One-time costs bypass the
Leontief pass" but the implementation now applies the Leontief inverse to once
when data.effects.cf_once is present (see variables/functions: once,
data.effects.cf_once, _leontief, _apply_leontief); update the module docstring
(the paragraph around lines 7-9) to state that temporal, periodic and once
domains all support cross-effect propagation via the Leontief inverse (or
rephrase to reflect that one-time costs no longer bypass the Leontief pass) so
the documentation matches the code behavior.
---
Nitpick comments:
In `@tests/math/test_contributions.py`:
- Line 143: Add a new unit test exercising the new cross_once behavior: mirror
the existing test_sizing_cross_effect_investment but use Effect(...,
cross_once={'co2': <value>}) and ensure the contributions pipeline applies
cf_once (the Leontief one-time cross-effect) from model.py/contributions.py,
asserting the one-time embodied emissions are added to CAPEX/sizing
contributions as expected; reference the Effect creation with cross_once, the
cf_once application path in contributions.py, and the model's handling of
cf_once to locate where to validate the result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5c46a7e-ec63-417d-8b03-b3ca822364aa
📒 Files selected for processing (12)
docs/guide/effects.mddocs/math/effects.mddocs/math/notation.mdsrc/fluxopt/contributions.pysrc/fluxopt/elements.pysrc/fluxopt/model.pysrc/fluxopt/model_data.pytests/math/test_contributions.pytests/math/test_effects.pytests/math_port/test_effects.pytests/math_port/test_multi_period.pytests/test_io.py
Comment on lines
+404
to
+407
| # Cross-effects on once via Leontief inverse | ||
| if data.effects.cf_once is not None: | ||
| once = _apply_leontief(_leontief(data.effects.cf_once), once) | ||
|
|
There was a problem hiding this comment.
Module docstring is now inconsistent with the implementation.
The module docstring (lines 7-9) states: "One-time costs bypass the Leontief pass". However, this code now applies the Leontief inverse to once when cf_once is present. The docstring should be updated to reflect that all three domains (temporal, periodic, once) now support cross-effect propagation via Leontief inverse.
📝 Suggested docstring fix (lines 7-9)
-Cross-effects use the Leontief inverse: total = (I - C)^-1 * direct,
-where C is the cross-effect coefficient matrix. One-time costs bypass
-the Leontief pass (matching the solver's ``effect_once`` variable).
+Cross-effects use the Leontief inverse: total = (I - C)^-1 * direct,
+where C is the cross-effect coefficient matrix. All three domains
+(temporal, periodic, once) apply Leontief when cross-effects are defined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fluxopt/contributions.py` around lines 404 - 407, The module docstring
claims "One-time costs bypass the Leontief pass" but the implementation now
applies the Leontief inverse to once when data.effects.cf_once is present (see
variables/functions: once, data.effects.cf_once, _leontief, _apply_leontief);
update the module docstring (the paragraph around lines 7-9) to state that
temporal, periodic and once domains all support cross-effect propagation via the
Leontief inverse (or rephrase to reflect that one-time costs no longer bypass
the Leontief pass) so the documentation matches the code behavior.
FBumann
changed the base branch from
fix/pr99-review-comments
to
feat/component-status-piecewise
Replace `contribution_from` / `contribution_from_per_hour` with explicit per-domain fields on Effect: - `cross_periodic` — sizing + recurring investment (Leontief on effect_periodic) - `cross_temporal` — per-timestep operational (Leontief on effect_temporal) - `cross_once` — one-time investment costs (Leontief on effect_once) This enables pricing embodied emissions from investment through cross-effects (e.g., construction CO2 → carbon cost), which was previously impossible. BREAKING CHANGE: `Effect.contribution_from` and `Effect.contribution_from_per_hour` are removed. The old `contribution_from` set both periodic and temporal matrices; the new API requires setting each domain explicitly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix module docstring: all three domains now support Leontief, not just temporal and periodic - Add TestCrossOnce with test verifying embodied CO2 priced into cost via cross_once Leontief pass Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove per-field unknown-effect checks in model_data.py (dead code — the early cycle-detection check catches unknown effects first). Add test_cross_effect_unknown_effect_raises to cover the early check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
FBumann
force-pushed
the
refac/cross-effect-api
branch
from
73e30c0 to
0272c27
Compare
7 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
Effect.contribution_from→Effect.cross_periodicandEffect.contribution_from_per_hour→Effect.cross_temporalEffect.cross_oncefor one-time investment cross-effects (e.g., embodied CO2 → carbon cost)cf_onceLeontief pass in both solver (model.py) and attribution (contributions.py)Closes #105.
BREAKING CHANGE:
contribution_fromandcontribution_from_per_hourremoved. The oldcontribution_fromset both periodic and temporal matrices; the new API requires explicit per-domain fields.Test plan
uv run pytest tests/ -q— 442 passed, 179 skipped, 11 xfaileduv run mypy src/— no issuesuv run ruff check .— all checks passedtest_cross_once_prices_embodied_emissions— verifies Investment CO2 priced into cost viacross_once🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation