Three bug fixes#292
Merged
ParticularlyPythonicBS merged 6 commits intoTemoaProject:unstablefrom Apr 8, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughCentralized end-of-life calculations and adjusted retirement-capacity bookkeeping in capacity logic; corrected demand indexing; migrated several manifest validators and removed custom hybrid loader methods; added new RTV-vintage viability sets and updated viability filtering; small enum base-type changes to StrEnum. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![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/db_schema/temoa_schema_v4.sql (1)
211-221:⚠️ Potential issue | 🟠 MajorVersion this persisted schema change before release.
Adding
periodtodemand_specific_distributionchanges the on-disk table contract, but this schema still reportsDB_MAJOR=4/DB_MINOR=0. Older 4.0 databases without this column will now look compatible until the loader intemoa/data_io/component_manifest.pystarts selectingperiodfrom this table, then fail at runtime. Please bump the schema version and mirror that bump in the shipped SQL assets, or add an explicit migration path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/db_schema/temoa_schema_v4.sql` around lines 211 - 221, This change adds the period column to the demand_specific_distribution table but leaves DB_MAJOR=4/DB_MINOR=0, so bump the persisted schema version (increment DB_MINOR, e.g., to 1) and update the shipped SQL assets to reflect the new version; ensure the loader in component_manifest.py that now selects period matches the new schema version (or add an explicit migration path) so older 4.0 databases without the new period column are not treated as compatible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/_internal/table_data_puller.py`:
- Around line 191-194: The branch that chooses between
model.demand_specific_distribution[r, p, s, d, o] and model.segment_fraction[s,
d] should be extracted into a single helper (e.g.,
get_annual_distribution(model, r, p, s, d, o)) that returns the correct
distribution value; implement this helper to check membership (o in
model.commodity_demand) and call value(...) on the appropriate expression,
replace the inline branch in table_data_puller.py (and other modules) with calls
to get_annual_distribution, and update imports/usage so future schema/index
changes are made in one place.
In `@temoa/components/capacity.py`:
- Around line 335-341: The last pre-EOL retirement period is excluded because
both guards use '<' against eol_year; update the comparisons in the
annual_retirement_constraint logic so the boundary case where p +
value(model.period_length[p_next]) == eol_year is included (i.e., use '<='
instead of '<' for the check that sets cap_end when computing p_next_end and for
the cap_begin check), ensuring v_retired_capacity[r, p_next, t, v] and
v_retired_capacity[r, p, t, v] are added back for the final pre-EOL retirement
period referenced by retired_capacity_variable_indices().
---
Outside diff comments:
In `@temoa/db_schema/temoa_schema_v4.sql`:
- Around line 211-221: This change adds the period column to the
demand_specific_distribution table but leaves DB_MAJOR=4/DB_MINOR=0, so bump the
persisted schema version (increment DB_MINOR, e.g., to 1) and update the shipped
SQL assets to reflect the new version; ensure the loader in
component_manifest.py that now selects period matches the new schema version (or
add an explicit migration path) so older 4.0 databases without the new period
column are not treated as compatible.
🪄 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: 6a0c3416-68fc-4531-9fb9-76b995e7521b
📒 Files selected for processing (16)
temoa/_internal/table_data_puller.pytemoa/components/capacity.pytemoa/components/commodities.pytemoa/components/emissions.pytemoa/components/reserves.pytemoa/core/model.pytemoa/data_io/component_manifest.pytemoa/data_io/hybrid_loader.pytemoa/db_schema/temoa_schema_v4.sqltemoa/model_checking/commodity_network_manager.pytemoa/tutorial_assets/utopia.sqltests/testing_data/mediumville.sqltests/testing_data/seasonal_storage.sqltests/testing_data/storageville.sqltests/testing_data/test_system.sqltests/testing_data/utopia_data.sql
Comment on lines
191
to
194
| if o in model.commodity_demand: | ||
| distribution = value(model.demand_specific_distribution[r, s, d, o]) | ||
| distribution = value(model.demand_specific_distribution[r, p, s, d, o]) | ||
| else: | ||
| distribution = value(model.segment_fraction[s, d]) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the annual distribution lookup into a shared helper.
This period-aware demand_specific_distribution/segment_fraction branch is now duplicated across multiple modules. Centralizing it would make the next schema/index change one-touch instead of another multi-file sweep.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/_internal/table_data_puller.py` around lines 191 - 194, The branch that
chooses between model.demand_specific_distribution[r, p, s, d, o] and
model.segment_fraction[s, d] should be extracted into a single helper (e.g.,
get_annual_distribution(model, r, p, s, d, o)) that returns the correct
distribution value; implement this helper to check membership (o in
model.commodity_demand) and call value(...) on the appropriate expression,
replace the inline branch in table_data_puller.py (and other modules) with calls
to get_annual_distribution, and update imports/usage so future schema/index
changes are made in one place.
ParticularlyPythonicBS
force-pushed
the
tbf
branch
from
51e2e7c to
5d43bf6
Compare
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 `@temoa/components/capacity.py`:
- Around line 316-318: Change the "else: if" pattern to an "elif" to reduce
indentation and improve readability: replace the block starting with "else:"
followed by "if p == model.time_optimize.last() or p_end == eol_year" with a
single "elif p == model.time_optimize.last() or p_end == eol_year" and adjust
the following else branch accordingly so control flow remains identical (keep
checks using p, model.time_optimize.last(), p_end, and eol_year and preserve the
logic for mid-life period and ending capacity assignment).
🪄 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: 44e65954-2bcd-4c9b-9cb6-6e4db9c05056
📒 Files selected for processing (4)
temoa/components/capacity.pytemoa/data_io/component_manifest.pytemoa/data_io/hybrid_loader.pytemoa/model_checking/commodity_network_manager.py
ParticularlyPythonicBS
merged commit 6f3f250
into
TemoaProject:unstable
12 checks passed
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
2 participants
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.
tech_or_groupcolumn, not atechcolumn, as this is now supported.ALSO: finally clean up the (str, Enum) type inheritance that ruff was upset about
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Style