Skip to content

Conversation

@ParticularlyPythonicBS
Copy link
Member

@ParticularlyPythonicBS ParticularlyPythonicBS commented Dec 18, 2025

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

  • Refactor
    • Consolidated seasonal configuration data into a unified structure for improved organization and maintainability
    • Enhanced temporal data modeling with refined seasonal segmentation capabilities across multiple periods
    • Updated database migration tools to ensure seamless compatibility with the revised schema

✏️ Tip: You can customize this high-level summary in your review settings.

@ParticularlyPythonicBS ParticularlyPythonicBS added Maintenance Code quality fixes and deprecation management database-schema labels Dec 18, 2025
@coderabbitai
Copy link

coderabbitai bot commented Dec 18, 2025

Walkthrough

This pull request consolidates the temporal-season modeling structure across multiple database schema files and migration utilities. The changes remove two legacy tables (time_season_all and time_season_to_sequential), replace them with a unified time_season_sequential table, and update corresponding migration mappings. Some database files also introduce a new time_segment_fraction table to support segmentation by time of day.

Changes

Cohort / File(s) Summary
Schema consolidation: Remove legacy tables + Add time_season_sequential
data_files/example_dbs/materials.sql, data_files/example_dbs/survival_curve.sql, data_files/example_dbs/test_system.sql, tests/testing_data/annualised_demand.sql, tests/testing_data/materials.sql, tests/testing_data/mediumville.sql, tests/testing_data/seasonal_storage.sql, tests/testing_data/simple_linked_tech.sql, tests/testing_data/storageville.sql, tests/testing_data/survival_curve.sql, tests/testing_data/test_system.sql
Removes time_season_all and time_season_to_sequential tables; introduces unified time_season_sequential table with seas_seq as TEXT, composite primary key on (period, sequence, seas_seq, season), num_days REAL NOT NULL, and CHECK (num_days > 0).
Schema consolidation with segment fractions
data_files/example_dbs/morris_utopia.sql, data_files/example_dbs/utopia.sql
Removes legacy tables and adds both time_season_sequential (unified schema) and time_segment_fraction (period, season, tod, segment_fraction, notes; primary key (period, season, tod); CHECK segment_fraction ∈ [0,1]) with associated data insertions.
Schema removal with segment retention
data_files/example_dbs/seasonal_storage.sql, tests/testing_data/emissions.sql
Removes time_season_all and time_season_to_sequential; retains/adds time_segment_fraction table with time-of-day segmentation data.
Schema removal only
data_files/temoa_schema_v4.sql, temoa/db_schema/temoa_schema_v4.sql
Removes time_season_all and time_season_to_sequential table definitions from schema without adding replacements.
Migration mapping updates
temoa/utilities/db_migration_v3_1_to_v4.py, temoa/utilities/sql_migration_v3_1_to_v4.py
Updates CUSTOM_MAP public dictionary: 'time_season' now maps to 'time_season' (was 'time_season_all'); 'time_season_sequential' now maps to 'time_season_sequential' (was 'time_season_to_sequential').

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Scope: 20+ files across multiple database schemas and migration utilities
  • Homogeneity: High degree of repetition (same consolidation pattern applied consistently across SQL files) reduces per-file review complexity
  • Key areas requiring attention:
    • Verify that the primary key structure (period, sequence, seas_seq, season) is consistent across all modified SQL files
    • Confirm the migration mapping updates align with the new table names and will not break downstream data migrations
    • Validate CHECK constraints (num_days > 0 and segment_fraction ∈ [0,1]) are applied correctly where applicable
    • Cross-check that files in data_files/ and tests/testing_data/ follow the same schema consolidation pattern

Possibly related PRs

  • PEP8 consistent model naming #190: Related schema-level changes to time/season objects; also removes/replaces time_season_all and time_season_to_sequential and updates references to consolidated structures.
  • adding database migrators to the cli #196: Related to database migration workflow; exposes the migration utilities (updated in this PR) via CLI interface, so schema consolidation and mapping changes directly impact downstream migration functionality.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the main change: removing two redundant time season tables from the schema.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch schema/remove_derivable_tables

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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, but S1 is not defined in season_label in this file

The new time_season_sequential and time_segment_fraction definitions 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_label without inserting an S1 row, while both time_season and time_segment_fraction reference S1. If foreign‑key checks are enabled when loading annualised_demand.sql, those inserts will violate the FK. Please confirm whether S1 is created elsewhere for this DB or add an INSERT INTO season_label('S1', ...) here.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 06ec2e7 and 2ebdc49.

📒 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 redundant time_season_all and time_season_to_sequential tables 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_season with existing data
  • Defines time_season_sequential with the new unified structure (including seas_seq and num_days columns with CHECK (num_days > 0))
  • Includes time_segment_fraction for segment-level fractions

The empty time_season_sequential table 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_season with period-specific data
  • Defines the unified time_season_sequential structure (empty, as expected for non-sequential storage scenarios)
  • Includes time_segment_fraction with 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_sequential structure. 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_days values (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_season retains data for all periods and seasons
  • time_season_sequential is empty (appropriate for non-sequential storage scenarios)
  • time_segment_fraction has complete coverage (48 entries: 3 periods × 4 seasons × 4 tod) with uniform fractions (0.0625) that sum to 1.0 per period
tests/testing_data/mediumville.sql (1)

1193-1222: Seasonal sequencing and segment fractions look consistent

time_season_sequential mirrors the expected shape of the old time_season_to_sequential (keys and num_days > 0 check), and time_segment_fraction has 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_sequential uses the standard (period, sequence, seas_seq, season, num_days) design with appropriate PK and CHECK, and the time_segment_fraction entries 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‑formed

The time_season_sequential schema matches the consolidated definition, and the time_segment_fraction entries 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_sequential uses the standard schema, and the time_segment_fraction rows 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 coherent

The time_season_sequential schema 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_sequential schema and data are internally consistent

  • Column set, PK, and CHECK constraint align with the new unified design.
  • For period 2000, num_days across all rows sum to 365, matching days_per_period.

1115-1135: time_segment_fraction rows form a normalized partition

  • FKs to time_period, season_label, and time_of_day are 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: Empty time_season_sequential table – confirm this is intentional for this DB

The unified time_season_sequential schema looks correct and matches other files, but there are no rows inserted here. If the previous time_season_all / time_season_to_sequential tables 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 from time_season + time_segment_fraction.


1545-1581: time_segment_fraction values are coherent with the seasonal structure

  • FKs line up with season_label, time_of_day, and time_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: Unpopulated time_season_sequential in materials test DB – double‑check test intent

The schema here matches the new unified time_season_sequential definition, but the table is left empty. If earlier versions had data in time_season_all / time_season_to_sequential that 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_fraction grid is well-formed and period‑normalized

  • All referenced seasons (summer, autumn, winter, spring) and TODs (morningovernight) 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‑empty time_season_sequential

The time_season_sequential schema 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 via time_season_to_sequential) and that any derivation logic now works against an empty table.


1545-1581: Test time_segment_fraction data correctly exercise seasonal/TOD structure

  • FKs and labels match the earlier season_label and time_of_day tables.
  • 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_fraction table 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_fraction are 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 populate time_season_sequential table in utopia.sql

The time_season_sequential table 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.

@ParticularlyPythonicBS ParticularlyPythonicBS merged commit 32a21e6 into unstable Dec 18, 2025
9 checks passed
@ParticularlyPythonicBS ParticularlyPythonicBS deleted the schema/remove_derivable_tables branch December 18, 2025 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

database-schema Maintenance Code quality fixes and deprecation management

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants