-
Notifications
You must be signed in to change notification settings - Fork 61
Updating schema to remove redundant time season tables #238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updating schema to remove redundant time season tables #238
Conversation
WalkthroughThis pull request consolidates the temporal-season modeling structure across multiple database schema files and migration utilities. The changes remove two legacy tables ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/testing_data/annualised_demand.sql (1)
1045-1080: Schema is fine, butS1is not defined inseason_labelin this fileThe new
time_season_sequentialandtime_segment_fractiondefinitions look consistent with the rest of the schema, and the single segment fraction row (2000,'S1','D1',1.0) is valid.However, this file defines
season_labelwithout inserting anS1row, while bothtime_seasonandtime_segment_fractionreferenceS1. If foreign‑key checks are enabled when loadingannualised_demand.sql, those inserts will violate the FK. Please confirm whetherS1is created elsewhere for this DB or add anINSERT INTO season_label('S1', ...)here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
data_files/example_dbs/materials.sql(2 hunks)data_files/example_dbs/morris_utopia.sql(2 hunks)data_files/example_dbs/seasonal_storage.sql(2 hunks)data_files/example_dbs/survival_curve.sql(2 hunks)data_files/example_dbs/test_system.sql(2 hunks)data_files/example_dbs/utopia.sql(2 hunks)data_files/temoa_schema_v4.sql(0 hunks)temoa/db_schema/temoa_schema_v4.sql(0 hunks)temoa/utilities/db_migration_v3_1_to_v4.py(1 hunks)temoa/utilities/sql_migration_v3_1_to_v4.py(2 hunks)tests/testing_data/annualised_demand.sql(2 hunks)tests/testing_data/emissions.sql(2 hunks)tests/testing_data/materials.sql(2 hunks)tests/testing_data/mediumville.sql(2 hunks)tests/testing_data/seasonal_storage.sql(1 hunks)tests/testing_data/simple_linked_tech.sql(2 hunks)tests/testing_data/storageville.sql(2 hunks)tests/testing_data/survival_curve.sql(2 hunks)tests/testing_data/test_system.sql(2 hunks)
💤 Files with no reviewable changes (2)
- data_files/temoa_schema_v4.sql
- temoa/db_schema/temoa_schema_v4.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
- GitHub Check: setup and test (ubuntu-latest, 3.13)
🔇 Additional comments (22)
temoa/utilities/db_migration_v3_1_to_v4.py (1)
21-36: LGTM! Migration mappings correctly updated for schema consolidation.The identity mappings (
'time_season': 'time_season'and'time_season_sequential': 'time_season_sequential') ensure that v3.1 databases using these table names migrate correctly to v4, where the redundanttime_season_allandtime_season_to_sequentialtables have been removed.temoa/utilities/sql_migration_v3_1_to_v4.py (1)
29-48: LGTM! SQL migration mappings are consistent with the SQLite migration utility.The mapping updates mirror those in
db_migration_v3_1_to_v4.py, ensuring both migration paths handle the schema consolidation identically. The'segfrac': 'segment_fraction'entry at line 38 appropriately handles case-insensitive mapping for the segment fraction column.tests/testing_data/simple_linked_tech.sql (1)
1050-1087: LGTM! Schema correctly consolidates time-related tables.The schema properly:
- Retains
time_seasonwith existing data- Defines
time_season_sequentialwith the new unified structure (includingseas_seqandnum_dayscolumns withCHECK (num_days > 0))- Includes
time_segment_fractionfor segment-level fractionsThe empty
time_season_sequentialtable is appropriate for this test case since it tests linked technologies rather than sequential seasonal storage.data_files/example_dbs/survival_curve.sql (1)
1164-1209: LGTM! Schema changes align with the PR objective.The survival curve example database correctly:
- Maintains
time_seasonwith period-specific data- Defines the unified
time_season_sequentialstructure (empty, as expected for non-sequential storage scenarios)- Includes
time_segment_fractionwith appropriate fractions for the single season 's'data_files/example_dbs/seasonal_storage.sql (1)
1078-1135: LGTM! Seasonal storage example demonstrates the new unified schema effectively.This file is the key demonstration of the consolidated
time_season_sequentialstructure. The data correctly:
- Maps sequential season identifiers (
seas_seq) like 'summer', 'sept_w1', 'winter' to their corresponding non-sequential seasons ('charge'/'discharge')- Specifies appropriate
num_daysvalues (152.5 for summer/winter, 7.0 for weekly segments, 1.0 for individual days)- Satisfies the
CHECK (num_days > 0)constraint- Provides a realistic seasonal storage modeling scenario with 14 sequential time slices
data_files/example_dbs/materials.sql (1)
1531-1624: LGTM! Materials example correctly implements the consolidated schema.The schema changes are consistent with the PR objective:
time_seasonretains data for all periods and seasonstime_season_sequentialis empty (appropriate for non-sequential storage scenarios)time_segment_fractionhas complete coverage (48 entries: 3 periods × 4 seasons × 4 tod) with uniform fractions (0.0625) that sum to 1.0 per periodtests/testing_data/mediumville.sql (1)
1193-1222: Seasonal sequencing and segment fractions look consistent
time_season_sequentialmirrors the expected shape of the oldtime_season_to_sequential(keys andnum_days > 0check), andtime_segment_fractionhas valid 0–1 fractions whose entries for 2025 sum to 1 across all season/tod combinations, with all referenced seasons and TODs defined earlier. No issues spotted here.tests/testing_data/emissions.sql (1)
1090-1128: Consolidated seasonal schema is coherent here
time_season_sequentialuses the standard(period, sequence, seas_seq, season, num_days)design with appropriate PK and CHECK, and thetime_segment_fractionentries for each (period, S1) sum to 1 across TODs, with all FK targets (season_label,time_of_day) present. This looks correct and aligned with the broader consolidation.tests/testing_data/storageville.sql (1)
1079-1114: Season/time‑of‑day segmentation is well‑formedThe
time_season_sequentialschema matches the consolidated definition, and thetime_segment_fractionentries for 2025 (s1/s2 over d1–d5) are all within [0,1] and sum to 1 overall, with all seasons and TODs defined. No issues from this change.data_files/example_dbs/morris_utopia.sql (1)
1543-1586: Utopia seasonal partitioning is consistent and normalized
time_season_sequentialuses the standard schema, and thetime_segment_fractionrows for 1990/2000/2010 form a proper partition of each period (fractions all within [0,1] and summing to 1 per period, with valid season/TOD labels). This aligns cleanly with the PR’s seasonal consolidation.tests/testing_data/survival_curve.sql (1)
1164-1209: Single‑season survival‑curve setup is coherentThe
time_season_sequentialschema matches the consolidated design, and each period has a single(season='s', tod='d')segment with fraction 1.0, with matching season and TOD labels defined. This change looks correct.tests/testing_data/seasonal_storage.sql (2)
1089-1114:time_season_sequentialschema and data are internally consistent
- Column set, PK, and CHECK constraint align with the new unified design.
- For period 2000,
num_daysacross all rows sum to 365, matchingdays_per_period.
1115-1135:time_segment_fractionrows form a normalized partition
- FKs to
time_period,season_label, andtime_of_dayare satisfied.- For period 2000, the 8 rows (2 seasons × 4 TODs at 0.125) sum to 1, which is coherent with the modeled period share.
data_files/example_dbs/test_system.sql (2)
1533-1543: Emptytime_season_sequentialtable – confirm this is intentional for this DBThe unified
time_season_sequentialschema looks correct and matches other files, but there are no rows inserted here. If the previoustime_season_all/time_season_to_sequentialtables carried data that were actually used in this example, you may want either to migrate that data or ensure the loader now derives everything it needs fromtime_season+time_segment_fraction.
1545-1581:time_segment_fractionvalues are coherent with the seasonal structure
- FKs line up with
season_label,time_of_day, andtime_period.- For each period, the 8 entries (4 seasons × 2 TODs at 0.125) sum to 1, matching a simple 4‑season, 2‑slice representation.
tests/testing_data/materials.sql (2)
1456-1466: Unpopulatedtime_season_sequentialin materials test DB – double‑check test intentThe schema here matches the new unified
time_season_sequentialdefinition, but the table is left empty. If earlier versions had data intime_season_all/time_season_to_sequentialthat were relevant for these materials tests, please verify that tests no longer depend on explicit sequential rows and that the HybridLoader (or related logic) can operate with an empty table.
1468-1528:time_segment_fractiongrid is well-formed and period‑normalized
- All referenced seasons (
summer,autumn,winter,spring) and TODs (morning…overnight) exist in their respective lookup tables.- For each period (2000, 2010, 2020), the 16 entries (4 seasons × 4 TODs at 0.0625) sum to 1, giving a clean, uniform partition of the period.
tests/testing_data/test_system.sql (2)
1533-1543: Tests: ensure no expectations around non‑emptytime_season_sequentialThe
time_season_sequentialschema is consistent, but there are no INSERTs. Given this SQL seeds a test database, please confirm there are no tests assuming populated sequential seasons (e.g., formerly viatime_season_to_sequential) and that any derivation logic now works against an empty table.
1545-1581: Testtime_segment_fractiondata correctly exercise seasonal/TOD structure
- FKs and labels match the earlier
season_labelandtime_of_daytables.- For each modeled period, 8 rows (4 seasons × 2 TODs at 0.125) sum to 1, which should give deterministic, easy‑to‑reason‑about test behavior.
data_files/example_dbs/utopia.sql (3)
1553-1565: LGTM: Well-structured time segment fraction table.The
time_segment_fractiontable definition is correct with:
- Appropriate foreign key constraints to time_period, season_label, and time_of_day
- Proper CHECK constraint ensuring segment_fraction values are between 0 and 1
- Suitable primary key ensuring uniqueness per period/season/tod combination
1566-1583: LGTM: Time segment fractions are correctly distributed.The INSERT statements for
time_segment_fractionare correct:
- Fractions properly sum to 1.0 for each period (1990, 2000, 2010)
- Consistent seasonal distribution: winter (50%), inter (25%), summer (25%)
- Day/night ratios are consistent (2:1) within each season
- All values satisfy the CHECK constraint (0 ≤ segment_fraction ≤ 1)
1541-1551: Add INSERT statements to populatetime_season_sequentialtable in utopia.sqlThe
time_season_sequentialtable has been created but contains no data. All other example database files (seasonal_storage.sql, test_system.sql, materials.sql, etc.) include INSERT statements immediately after the table definition to populate this table. The utopia.sql file should follow the same pattern and include the necessary INSERT statements.
removed the time_season_all and time_season_to_sequential tables from the Temoa schema and example data. These tables were redundant because their data is already derived by the HybridLoader from the time_season and time_season_sequential tables.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.