Fix/myopic retirement#340
Conversation
updates: - [github.com/astral-sh/uv-pre-commit: 0.11.19 → 0.11.21](astral-sh/uv-pre-commit@0.11.19...0.11.21) - [github.com/astral-sh/ruff-pre-commit: v0.15.16 → v0.15.17](astral-sh/ruff-pre-commit@v0.15.16...v0.15.17)
updates: - [github.com/astral-sh/uv-pre-commit: 0.11.21 → 0.11.23](astral-sh/uv-pre-commit@0.11.21...0.11.23) - [github.com/astral-sh/ruff-pre-commit: v0.15.17 → v0.15.18](astral-sh/ruff-pre-commit@v0.15.17...v0.15.18)
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
WalkthroughThe PR adds retired-existing-capacity support for myopic runs across the model, time-period handling, data loading, and capacity calculations, and bumps two pre-commit hook revisions. ChangesMyopic capacity adjustments
Pre-commit revisions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 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)
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 |
|
Just opening this for the rabbit. Looks like it needs some rebasing |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/data_io/component_manifest.py (1)
448-448: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
validation_mapafter removing the validator.With
validator_nameremoved,_filter_datareturns early (no validator), sovalidation_map=(0, 2, 3)is never used. Consider dropping it to avoid implying validation that no longer happens.♻️ Suggested cleanup
LoadItem( component=model.lifetime_survival_curve, table='lifetime_survival_curve', columns=['region', 'period', 'tech', 'vintage', 'fraction'], - validation_map=(0, 2, 3), is_period_filtered=False, is_table_required=False, ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/data_io/component_manifest.py` at line 448, The validation_map argument is now dead because _filter_data no longer uses it once validator_name was removed, so the ComponentManifest-related call still implies validation that never happens. Update the relevant _filter_data invocation to drop validation_map=(0, 2, 3), and keep the remaining arguments consistent with the no-validator flow in component_manifest.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@temoa/components/time.py`:
- Around line 202-203: The branch in the time-period logic is unnecessarily
using an elif after a preceding return, which triggers RET505. Update the
conditional in the relevant function in time.py so the check after the earlier
return is expressed as a plain if/early-return style, keeping the same behavior
while removing the redundant elif.
In `@temoa/data_io/hybrid_loader.py`:
- Around line 639-642: The docstring on the method currently describes behavior
copied from `_load_existing_capacity` that this code does not perform. Update
the documentation for the affected method in `hybrid_loader.py` so it accurately
reflects this method’s actual purpose and remove references to handling myopic
vs. standard runs or populating `tech_exist`; use the surrounding method name
and its current implementation to rewrite the wording consistently.
---
Outside diff comments:
In `@temoa/data_io/component_manifest.py`:
- Line 448: The validation_map argument is now dead because _filter_data no
longer uses it once validator_name was removed, so the ComponentManifest-related
call still implies validation that never happens. Update the relevant
_filter_data invocation to drop validation_map=(0, 2, 3), and keep the remaining
arguments consistent with the no-validator flow in component_manifest.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10b82a90-3eef-45e4-8c3a-95a14a1189f5
📒 Files selected for processing (9)
.pre-commit-config.yamltemoa/components/capacity.pytemoa/components/limits.pytemoa/components/technology.pytemoa/components/time.pytemoa/components/utils.pytemoa/core/model.pytemoa/data_io/component_manifest.pytemoa/data_io/hybrid_loader.py
| elif p in model.time_exist: | ||
| return -1 # Period length is not defined for existing periods except the last |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Optional: collapse elif after return (RET505).
Since the preceding branch returns, the elif can be a plain if/early-return to satisfy the linter.
♻️ Suggested tweak
if model.time_exist and p == model.time_exist.last():
# Need this for one specific use case (capacity growth constraints)
return model.time_future.first() - model.time_exist.last()
- elif p in model.time_exist:
+ if p in model.time_exist:
return -1 # Period length is not defined for existing periods except the last📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif p in model.time_exist: | |
| return -1 # Period length is not defined for existing periods except the last | |
| if p in model.time_exist: | |
| return -1 # Period length is not defined for existing periods except the last |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 202-202: Unnecessary elif after return statement
Remove unnecessary elif
(RET505)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@temoa/components/time.py` around lines 202 - 203, The branch in the
time-period logic is unnecessarily using an elif after a preceding return, which
triggers RET505. Update the conditional in the relevant function in time.py so
the check after the earlier return is expressed as a plain if/early-return
style, keeping the same behavior while removing the redundant elif.
Source: Linters/SAST tools
| """ | ||
| Handles different queries for myopic vs. standard runs and also | ||
| populates the `tech_exist` set. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Fix copied docstring.
This is copy-pasted from _load_existing_capacity; this method neither handles a standard (non-myopic) run nor populates tech_exist.
✏️ Suggested wording
"""
- Handles different queries for myopic vs. standard runs and also
- populates the `tech_exist` set.
+ Loads retired existing capacity (cap_early) from prior planning periods.
+ Only active in myopic mode.
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """ | |
| Handles different queries for myopic vs. standard runs and also | |
| populates the `tech_exist` set. | |
| """ | |
| """ | |
| Loads retired existing capacity (cap_early) from prior planning periods. | |
| Only active in myopic mode. | |
| """ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@temoa/data_io/hybrid_loader.py` around lines 639 - 642, The docstring on the
method currently describes behavior copied from `_load_existing_capacity` that
this code does not perform. Update the documentation for the affected method in
`hybrid_loader.py` so it accurately reflects this method’s actual purpose and
remove references to handling myopic vs. standard runs or populating
`tech_exist`; use the surrounding method name and its current implementation to
rewrite the wording consistently.
2 participants
Early retirements were not being carried forward in myopic mode. This change adds a new hidden parameter that brings the results of
output_retired_capacity'scap_earlycolumn forward into future myopic periods. Then, we add a utility function to calculate the adjusted existing capacity, which is the existing capacity that was build minus early retirements. This version is compatible with myopic + early retirement + survival curves + growth rate constraints from existing capacity (nightmare fuel).Also solves a smaller bug where, ass the myopic sequencer moves forward, it checks whether previously built capacity survived thus far. If any existing capacity is not seen in the previous period's net capacity output (e.g., was retired early, or survival curved down under the threshold) then that process is removed from the myopic efficiency table and therefore is no longer a valid process going forward.
HOWEVER, these are still pulled into the existing capacity parameter for accounting purposes. This all works out fine except that I had previously put in a check that will not-so-helpfully tell users that their existing capacity should be a valid process but isn't (because previous myopic horizons decided to retire it early) then raise a ValueError and crash the run.
Changing this error to a warning fixes the issue. In perfect foresight, this should always be a bug but not necessarily a serious bug so a warning should cover that anyway.
Summary by CodeRabbit
New Features
Bug Fixes