feat: add build_period axis to investment once-effect matrices#98
Open
feat: add build_period axis to investment once-effect matrices#98
Conversation
Investment once-effects (effects_per_size, effects_fixed) now support a 2D (period, build_period) internal representation, enabling construction cost spread, learning curves, and installment plans. Construction rule: - Scalar → diagonal (constant cost lands in build period) - 1D (build_period,) → diagonal (cost varies by when you build) - 2D (period, build_period) → as-is (full control) model.py renames invest_size_at_build/invest_build period→build_period before multiplication, then sums over both flow and build_period. Closes #96 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughRefactors public input types and type hints (replacing TimeSeries with ArrayInput/Temporal/Periodic variants); adds build_period-aware once-effect expansion for investment effects (diagonal/2D rules), updates model aggregation to sum over build_period, and adds unit/integration tests for the new behavior. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![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
🤖 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/model_data.py`:
- Around line 83-94: The code paths handling once-effect DataArrays (in the
block using vdims, value, dims, coords, and n) must align/validate the DataArray
coordinates for 'period' and 'build_period' before using them; update the branch
where vdims == {'period','build_period'} and the branch that diagonalizes 1D
inputs so that you reindex or sel the incoming value to the expected
period/build_period coordinate ordering (using the target coords derived from
coords or dims), check the reindexed length matches n, and then construct the 2D
DataArray using those aligned values (instead of raw .values) so costs map to
the correct period/build_period labels; also include the same coordinate
alignment/validation before raising the ValueError that references dim_name and
lengths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95827c70-678d-4c89-9597-cb0d2b284796
📒 Files selected for processing (4)
src/fluxopt/elements.pysrc/fluxopt/model.pysrc/fluxopt/model_data.pytests/math_port/test_multi_period.py
Reindex 1D and 2D DataArray inputs to the target period/build_period coords so values map to the correct labels regardless of input ordering. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/fluxopt/model_data.py (1)
83-84:⚠️ Potential issue | 🟡 MinorAdd null check after reindexing 2D DataArray.
The 1D path (lines 87-92) validates for nulls after reindex, but the 2D path returns directly without checking. If the input
valuehasperiod/build_periodcoordinates that don't fully cover the model'speriodindex,reindexfills gaps with NaN, silently corrupting the cost matrix.🐛 Proposed fix
if vdims == {'period', 'build_period'}: - return value.reindex(period=period, build_period=period) + aligned = value.reindex(period=period, build_period=period) + if aligned.isnull().any(): + raise ValueError( + f'Once-effect DataArray (period, build_period) has coords that do not ' + f'fully cover the model periods {list(period)}' + ) + return aligned🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fluxopt/model_data.py` around lines 83 - 84, The 2D branch that handles vdims == {'period', 'build_period'} currently returns value.reindex(period=period, build_period=period) without validating for NaNs; change it to perform the reindex into a local variable (e.g., reindexed = value.reindex(period=period, build_period=period)), then mirror the 1D-path null check (e.g., if reindexed.isnull().any(): raise ValueError(...) or handle consistently) before returning reindexed so missing period/build_period coordinates don't silently produce NaNs in the cost matrix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/fluxopt/model_data.py`:
- Around line 83-84: The 2D branch that handles vdims == {'period',
'build_period'} currently returns value.reindex(period=period,
build_period=period) without validating for NaNs; change it to perform the
reindex into a local variable (e.g., reindexed = value.reindex(period=period,
build_period=period)), then mirror the 1D-path null check (e.g., if
reindexed.isnull().any(): raise ValueError(...) or handle consistently) before
returning reindexed so missing period/build_period coordinates don't silently
produce NaNs in the cost matrix.
Cover all branches: scalar, list, 1D build_period, 1D period, reordered coords alignment, 2D passthrough, 2D reordered, mismatched coords error, and foreign dim error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the misleading TimeSeries type alias with ArrayInput — it accepts
scalars, sequences, and DataArrays, not just time-indexed data.
Add dimension-annotated variants via Annotated for IDE hover hints:
- TemporalInput (dims ⊆ {time})
- TemporalPeriodicInput (dims ⊆ {time, period})
- PeriodicInput (dims ⊆ {period})
- OnceEffectInput (dims ⊆ {period, build_period})
Uses TypeAlias assignments (not PEP 695 type statements) so that
mkdocstrings/griffe renders them as linkable, documented members.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
_expand_once_effect()helper implementing the scalar→1D→diagonal→2D construction rule for investment once-effectseffects_per_sizeandeffects_fixednow carry(invest_dim, effect, period, build_period)shape internallymodel.pyrenamesinvest_size_at_build/invest_buildperiod→build_period before multiplication, then sums over bothflowandbuild_periodeffects_per_size_periodic,effects_fixed_periodic) unchanged — they don't have a build_period axisConstruction rule
5.0(build_period,)(period, build_period)Test plan
build_periodper-size, 1Dbuild_periodfixed, 2D spread per-size, 2D spread fixedruff check,ruff format,mypycleanCloses #96
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests