Consolidating test database creation #239
Conversation
ParticularlyPythonicBS
added
Maintenance
WalkthroughCentralized test-data paths and a DB builder were added to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 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 |
ParticularlyPythonicBS
force-pushed
the
ref/consolidating_testing_dbs
branch
from
dcd0faf to
0d148b6
Compare
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
tests/conftest.py(2 hunks)tests/testing_configs/config_annualised_demand.toml(1 hunks)tests/testing_configs/config_emissions.toml(1 hunks)tests/testing_configs/config_link_test.toml(1 hunks)tests/testing_configs/config_materials.toml(1 hunks)tests/testing_configs/config_mediumville.toml(1 hunks)tests/testing_configs/config_seasonal_storage.toml(1 hunks)tests/testing_configs/config_storageville.toml(1 hunks)tests/testing_configs/config_survival_curve.toml(1 hunks)tests/testing_configs/config_test_system.toml(1 hunks)tests/testing_configs/config_utopia.toml(1 hunks)tests/testing_configs/config_utopia_gv.toml(1 hunks)tests/testing_configs/config_utopia_myopic.toml(1 hunks)tests/testing_data/annualised_demand.sql(1 hunks)tests/testing_data/emissions.sql(1 hunks)tests/testing_data/mediumville.sql(1 hunks)tests/testing_data/seasonal_storage.sql(1 hunks)tests/testing_data/simple_linked_tech.sql(1 hunks)tests/testing_data/storageville.sql(1 hunks)tests/testing_data/survival_curve.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T22:55:02.519Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 186
File: temoa/_internal/temoa_sequencer.py:60-63
Timestamp: 2025-11-03T22:55:02.519Z
Learning: In `TemoaSequencer.__init__`, when a `mode_override` is provided, it must be written back to `self.config.scenario_mode` (not just stored in `self.temoa_mode`) because `HybridLoader.create_data_dict()` has strict consistency checks that validate `config.scenario_mode` matches the presence/absence of `myopic_index`. Without this synchronization, MYOPIC mode would fail with a RuntimeError when the loader is instantiated.
Applied to files:
tests/testing_configs/config_link_test.tomltests/testing_configs/config_storageville.tomltests/testing_configs/config_mediumville.toml
🪛 GitHub Actions: CI
tests/conftest.py
[error] 45-45: mypy check failed: Incompatible default for argument "modifications" (default has type "None", argument has type "list[tuple[str, tuple[Any, ...]]]"). Exit code due to no_implicit_optional upgrade; Command: 'uv run mypy --config-file=pyproject.toml .'
🪛 Ruff (0.14.8)
tests/conftest.py
45-45: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🔇 Additional comments (25)
tests/testing_data/mediumville.sql (1)
1-198: LGTM: Idempotent data loading pattern implemented correctly.The conversion to
REPLACE INTOstatements aligns with the PR objective of creating data-only SQL files that load into a centralized schema. This approach ensures test databases can be rebuilt repeatably without schema conflicts.tests/testing_data/simple_linked_tech.sql (1)
1-63: LGTM: Consistent upsert-based data loading.The file correctly implements the idempotent data loading pattern using
REPLACE INTOstatements, consistent with the broader PR refactoring to centralize schema management.tests/testing_data/annualised_demand.sql (1)
1-55: LGTM: Data-only file correctly structured.The conversion to
REPLACE INTOstatements successfully transforms this into a data-only file that can load repeatably into a centralized schema.tests/testing_configs/config_utopia_gv.toml (1)
10-11: LGTM: Configuration parameters consolidated at top level.The addition of
time_sequencingandreserve_marginas top-level configuration options aligns with the PR's goal of standardizing test configurations. The values are appropriate for test scenarios.tests/testing_configs/config_emissions.toml (1)
10-11: LGTM: Consistent configuration standardization.The addition of
time_sequencingandreserve_marginparameters follows the same pattern as other test configurations, promoting consistency across the test suite.tests/testing_configs/config_mediumville.toml (1)
10-11: LGTM: Configuration parameters properly standardized.The top-level
time_sequencingandreserve_marginparameters are correctly added, maintaining consistency with the refactored test infrastructure.tests/testing_data/emissions.sql (1)
1-104: LGTM: Idempotent data loading correctly implemented.The file successfully converts to the
REPLACE INTOpattern, enabling repeatable test database construction from centralized schema sources.tests/testing_configs/config_test_system.toml (1)
10-11: LGTM: Configuration standardization complete.The addition of
time_sequencingandreserve_margincompletes the consistent refactoring pattern across all test configuration files, supporting the centralized database creation approach.tests/testing_configs/config_utopia.toml (2)
10-11: LGTM!The addition of
time_sequencingandreserve_marginas top-level entries aligns with the consolidation effort across test configs. Values are consistent with the model's expected options.
13-21: MGA and myopic sections look good.The simplified, comment-free format is consistent with other test configs in this PR.
tests/testing_configs/config_annualised_demand.toml (1)
10-11: LGTM!The use of
time_sequencing = "representative_periods"(versus"seasonal_timeslices"in other configs) appears intentional for the annualized demand test scenario. The configuration is consistent with the consolidation pattern.tests/testing_configs/config_storageville.toml (2)
10-11: LGTM!Top-level
time_sequencingandreserve_marginentries are consistent with the consolidation pattern.
13-19: Verify MGA section parameter differences.This config uses
slack,iterations, andweightfor MGA, while other configs (e.g.,config_utopia.toml,config_annualised_demand.toml) usecost_epsilon,iteration_limit,time_limit_hrs,axis, andweighting. Please verify this difference is intentional for the storageville test scenario.tests/testing_configs/config_survival_curve.toml (1)
10-21: LGTM!Configuration changes are consistent with the consolidation pattern. Top-level
time_sequencingandreserve_marginentries, along with simplified MGA and myopic sections, align with other test configs.tests/testing_configs/config_link_test.toml (2)
13-14: LGTM!Top-level
time_sequencingandreserve_marginentries are consistent with the consolidation pattern.
21-23: Verify myopic section parameters.This config uses
view_depthandstep_size, while other configs (e.g.,config_utopia.toml,config_storageville.toml) usemyopic_view. Please verify this is the correct parameter set for the link_test scenario and that it aligns with the expectedTemoaSequencer/HybridLoaderconfiguration requirements. Based on learnings, myopic-related config must be consistent with mode validation checks.tests/testing_data/storageville.sql (1)
1-90: LGTM - Idempotent data loading approach.The conversion from schema-creating INSERTs to
REPLACE INTOstatements aligns with the PR objective of separating schema from data. This allows test data files to be re-run without conflicts and supports the centralized schema loading pattern introduced inconftest.py.Data integrity checks:
demand_specific_distributionfractions sum to 1.0 ✓time_segment_fractionvalues sum to 1.0 ✓tests/testing_data/survival_curve.sql (1)
1-185: LGTM! Data-only seeding pattern is correct.The conversion from schema creation to idempotent
REPLACE INTOstatements aligns well with the PR objective of centralizing schema management. This file now serves purely as a data-seeding script, relying on the central schema loaded by_build_test_db()inconftest.py.tests/testing_data/seasonal_storage.sql (1)
1-111: LGTM! Consistent data-only seeding pattern.The refactoring mirrors the approach in
survival_curve.sql, converting to idempotentREPLACE INTOstatements that depend on the central schema. This is the correct pattern for the consolidated database creation approach.tests/conftest.py (3)
36-39: LGTM! Well-organized central paths.The introduction of
TEST_DATA_PATH,TEST_OUTPUT_PATH, andSCHEMA_PATHconstants provides clear, centralized path management for the test infrastructure.
70-95: LGTM! Clean refactoring to centralized database building.The refactored
refresh_databases()function now uses the new_build_test_db()helper, eliminating duplication and centralizing the database creation logic. The approach of building all test databases from a central schema plus data scripts is much cleaner.
97-168: LGTM! Consistent use of centralized database builder.The refactored
create_unit_test_dbs()function leverages the new_build_test_db()helper to generate test databases fromutopia_data.sqlwith variations. This is a significant improvement in maintainability and consistency.tests/testing_configs/config_seasonal_storage.toml (1)
10-11: LGTM! Standardized configuration structure.The addition of top-level
time_sequencingandreserve_marginsettings standardizes the configuration structure across test files. This improves consistency and aligns with the broader refactoring in this PR.tests/testing_configs/config_materials.toml (1)
10-11: LGTM! Consistent configuration standardization.The top-level
time_sequencingandreserve_marginsettings align with the standardization pattern applied across all test configuration files in this PR.tests/testing_configs/config_utopia_myopic.toml (1)
10-11: LGTM! Configuration standardization with appropriate myopic settings.The top-level
time_sequencingandreserve_marginsettings standardize the configuration, while the myopic-specific controls (view_depthandstep_size) are appropriately configured for the myopic test scenario.
ParticularlyPythonicBS
force-pushed
the
ref/consolidating_testing_dbs
branch
from
0d148b6 to
71e9fea
Compare
ParticularlyPythonicBS
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tests/testing_configs/config_materials.toml (1)
16-16: Removed inline comments - verify documentation.Similar to
config_mediumville.toml, the inline comments have been removed. Ensure that valid option values are documented elsewhere.Also applies to: 19-19
tests/conftest.py (1)
42-68: Fix the type annotation (already flagged in previous review).The type annotation issue on line 45 was already identified in the previous review and should be addressed as specified.
Based on past review comments, this must be fixed to resolve the mypy pipeline failure.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
tests/conftest.py(2 hunks)tests/testing_configs/config_annualised_demand.toml(1 hunks)tests/testing_configs/config_emissions.toml(1 hunks)tests/testing_configs/config_link_test.toml(1 hunks)tests/testing_configs/config_materials.toml(1 hunks)tests/testing_configs/config_mediumville.toml(1 hunks)tests/testing_configs/config_seasonal_storage.toml(1 hunks)tests/testing_configs/config_storageville.toml(1 hunks)tests/testing_configs/config_survival_curve.toml(1 hunks)tests/testing_configs/config_test_system.toml(1 hunks)tests/testing_configs/config_utopia.toml(1 hunks)tests/testing_configs/config_utopia_gv.toml(1 hunks)tests/testing_configs/config_utopia_myopic.toml(1 hunks)tests/testing_data/annualised_demand.sql(1 hunks)tests/testing_data/emissions.sql(1 hunks)tests/testing_data/mediumville.sql(1 hunks)tests/testing_data/seasonal_storage.sql(1 hunks)tests/testing_data/simple_linked_tech.sql(1 hunks)tests/testing_data/storageville.sql(1 hunks)tests/testing_data/survival_curve.sql(1 hunks)tests/testing_data/utopia_data.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T22:55:02.519Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 186
File: temoa/_internal/temoa_sequencer.py:60-63
Timestamp: 2025-11-03T22:55:02.519Z
Learning: In `TemoaSequencer.__init__`, when a `mode_override` is provided, it must be written back to `self.config.scenario_mode` (not just stored in `self.temoa_mode`) because `HybridLoader.create_data_dict()` has strict consistency checks that validate `config.scenario_mode` matches the presence/absence of `myopic_index`. Without this synchronization, MYOPIC mode would fail with a RuntimeError when the loader is instantiated.
Applied to files:
tests/testing_configs/config_storageville.tomltests/testing_configs/config_link_test.tomltests/testing_configs/config_mediumville.toml
🪛 Ruff (0.14.8)
tests/conftest.py
45-45: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
⏰ 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). (1)
- GitHub Check: setup and test (macos-latest, 3.13)
🔇 Additional comments (34)
tests/testing_configs/config_emissions.toml (4)
3-9: Inconsistency: Keys claimed as removed are still present.The AI summary states that
input_database,output_database,neos,solver_name,save_excel,save_duals, andsave_lp_filewere removed from this configuration file. However, these keys are still present in lines 3-9. Please verify whether these keys should remain or be removed as part of the database consolidation effort.
10-11: LGTM: Standardized configuration parameters added.The addition of top-level
time_sequencingandreserve_marginparameters aligns with the PR's objective to standardize test configurations across the suite.
16-16: Simplified MGA weight configuration.The
weightparameter has been set to"integer"with the inline comment removed. This simplification improves readability.
19-19: Simplified myopic configuration.The
myopic_viewparameter has been set to2with the inline comment removed. This simplification improves readability.tests/testing_configs/config_survival_curve.toml (2)
10-11: LGTM!The addition of top-level
time_sequencingandreserve_marginconfiguration keys aligns with the PR's goal to standardize test configurations.
14-21: LGTM!The MGA and myopic section parameters are well-formatted and contain reasonable values for testing purposes. The condensed formatting improves readability.
tests/testing_configs/config_storageville.toml (3)
16-16: LGTM: Clean parameter definition.The removal of the inline comment improves readability while preserving the configuration value.
19-19: LGTM: Clean parameter definition.The removal of the inline comment improves consistency and readability.
10-11: The config parser correctly recognizes these top-level keys and all test configurations are updated consistently.The TOML parser in
temoa/core/config.pyaccepts bothtime_sequencingandreserve_marginas top-level parameters. The config data is properly loaded viatemoa/data_io/hybrid_loader.pyinto the model, and all 12 test configuration files have been updated to include both keys at the top level. The model components actively use these parameters, and no orphaned references to old locations remain.tests/testing_configs/config_utopia_gv.toml (2)
3-9: AI summary incorrectly claims these configuration options were removed.The AI-generated summary states that
input_database,output_database,neos,solver_name,save_excel,save_duals, andsave_lp_filewere removed from this file. However, these configuration options are still present in the code (lines 3-9). The provided code shows the final state after changes, and these keys remain unchanged in this file.
10-11: Configuration values are valid and consistent with Temoa system.Both
time_sequencing = "seasonal_timeslices"andreserve_margin = "static"are supported values in the Temoa system. These values are used consistently across the test suite and are explicitly handled in the codebase via dedicated case statements and functions (e.g.,reserve_margin_static()intemoa/components/reserves.py).tests/testing_configs/config_annualised_demand.toml (2)
3-9: Inconsistency between AI summary and code.The AI summary states that
input_database,output_database,neos,solver_name,save_excel,save_duals, andsave_lp_filewere removed from this file. However, these options are still present in the provided code (lines 3-9).
10-11: No issues found. Bothtime_sequencing = "representative_periods"andreserve_margin = "static"are valid, supported configuration options correctly placed at the top level. The values are consistent with other test configurations and supported by the codebase.tests/testing_configs/config_mediumville.toml (2)
10-11: LGTM! Standardization of top-level configuration keys.Adding
time_sequencingandreserve_marginas top-level keys aligns with the PR's objective to consolidate configuration structure across test files.
16-16: Verify that removed inline comments are documented elsewhere.The inline comments (e.g.,
# currently supported: [integer, normalized]forweight) provided helpful information about valid options. Ensure this information is available in user-facing documentation or schema definitions.Also applies to: 19-19
tests/testing_configs/config_link_test.toml (2)
13-14: LGTM! Consistent standardization.The addition of top-level
time_sequencingandreserve_marginmaintains consistency with the broader PR refactoring.
22-23: No validation issue exists here. The configuration is correct. Having[myopic]parameters defined whilescenario_mode = "perfect_foresight"is valid—the myopic section is simply ignored during perfect foresight execution. Whenscenario_mode = "perfect_foresight",HybridLoaderis invoked withmyopic_index=None, which bypasses myopic-specific logic entirely.Additionally,
config_link_test.tomluses the correct parameter names (view_depthandstep_size), which are required byMyopicSequencer. Note that other test configs usemyopic_view, which is never referenced in the codebase and appears to be a legacy parameter name.tests/testing_configs/config_materials.toml (1)
10-11: LGTM! Consistent configuration standardization.The top-level
time_sequencingandreserve_marginkeys follow the same standardization pattern applied across multiple test configuration files in this PR.tests/testing_configs/config_utopia.toml (2)
14-18: LGTM! Cleaner MGA configuration structure.The explicit key-value format improves readability and maintainability compared to inline documentation style.
10-11: The configuration keystime_sequencingandreserve_marginare actively consumed by the application. Both keys are loaded intemoa/data_io/hybrid_loader.py(lines 265 and 267) and used throughout the codebase:time_sequencingis consumed intemoa/components/time.pyandtemoa/components/operations.pyto control temporal model behavior, whilereserve_marginis consumed intemoa/components/reserves.pyand validated intemoa/model_checking/validators.py.tests/testing_data/annualised_demand.sql (1)
1-55: Schema-first loading pattern applied consistently.This file follows the same pattern as other test data files in this PR: REPLACE INTO with trailing NULLs, no schema creation, no transactions. The systematic refactor aligns with the PR objective to centralize schema management.
Ensure the same schema coordination checks mentioned for simple_linked_tech.sql apply here:
- Central schema loaded before data
- Column counts match between schema and data
- Test isolation despite missing transaction boundaries
Based on the PR's consolidation objectives.
tests/testing_data/survival_curve.sql (1)
1-185: LGTM! Consistent data-only refactor for survival curve tests.The extensive dataset properly preserves scientific notation values (e.g., line 87:
8.80000000000000115e-01) and follows the standardized REPLACE INTO pattern. The survival curve data across multiple time periods and technologies appears comprehensive.tests/testing_configs/config_test_system.toml (1)
10-11: Consistent configuration standardization.Same time_sequencing and reserve_margin additions as in config_utopia.toml. Ensure these keys are validated across all test configurations for consistency.
tests/testing_data/utopia_data.sql (1)
1-559: LGTM! Comprehensive central test data source.This file serves as the single-source-of-truth for utopia test data, supporting the PR's consolidation objective. The data is comprehensive, covering:
- Multiple technologies across electric, residential, and transport sectors
- Time periods from 1960-2020 with seasonal/time-of-day granularity
- Cost structures (fixed, variable, investment)
- Efficiency, emissions, capacity, and demand specifications
The REPLACE INTO approach makes this data idempotent and suitable for reuse across multiple test scenarios.
Based on the PR's stated goal of centralizing test database construction.
tests/testing_data/seasonal_storage.sql (1)
1-111: LGTM! Specialized seasonal storage test data.The data properly captures the complexity of seasonal storage modeling:
- Sequential time periods (summer, weekly September/April segments, specific days)
- Storage duration parameters (4h daily, 8760h seasonal)
- Capacity factors varying by season and time-of-day for charge/discharge cycles
- Storage level fraction limits for both daily and seasonal storage
The use of time_season_sequential table (lines 90-103) is particularly appropriate for modeling storage state evolution across seasons.
tests/testing_data/mediumville.sql (1)
1-198: LGTM! Comprehensive multi-region test dataset.The mediumville data provides excellent coverage of advanced modeling features:
- Multi-region setup (A, B) with inter-regional transport
- Technology groups and RPS requirements
- Planning reserve margins and ramp rate constraints
- Linked technologies and storage systems
- Multiple demand types (lighting, heating)
This comprehensive test case will help validate the consolidated database loading approach across complex scenarios.
tests/testing_data/simple_linked_tech.sql (1)
1-63: Schema coordination is properly implemented; data-schema alignment and transaction handling are sound.The concerns about schema loading order, column consistency, and transaction atomicity do not apply here. The test database setup in
conftest.py(lines 42-67) explicitly:
- Loads the central schema first (
temoa_schema_v4.sql) viacon.executescript()at line 54- Then loads data scripts (including this file) at lines 59-61
- Enforces atomicity through
con.commit()at line 67All REPLACE INTO statements in this file have consistent column counts per table (verified: 4 columns for
commodity, 14 fortechnology, etc.), ensuring no runtime schema mismatches. The use of REPLACE INTO is appropriate and intentional for idempotent test data that may be reloaded across test runs. The explicit separation of schema initialization from data loading via the centralized_build_test_db()function prevents data/schema drift and maintains a single source of truth for the database structure.tests/testing_configs/config_seasonal_storage.toml (1)
10-11: LGTM! Configuration centralization improves maintainability.Moving
time_sequencingandreserve_marginto the top level centralizes these settings, making the configuration structure clearer and more consistent across test configs.tests/testing_configs/config_utopia_myopic.toml (1)
10-11: LGTM! Consistent configuration structure.The centralization of
time_sequencingandreserve_marginto top-level keys, along with the explicit myopic parameters, aligns with the PR's standardization objectives.Also applies to: 19-20
tests/testing_data/emissions.sql (1)
1-104: LGTM! Consistent idempotent data approach.The conversion to
REPLACE INTOfollows the same pattern as other data files in this PR, ensuring idempotent test data population. This works seamlessly with the centralized schema-loading approach inconftest.py.tests/conftest.py (3)
36-40: LGTM! Path centralization improves maintainability.Defining central path constants makes the test infrastructure more maintainable and reduces the risk of path-related errors.
70-94: LGTM! Clean refactoring with centralized DB construction.The refactored
refresh_databasesfunction effectively uses the new_build_test_dbhelper, eliminating code duplication and making the database construction process uniform across all test databases.
97-167: LGTM! Single-source approach with variations is elegant.Using a single source SQL file (
utopia_data.sql) with targeted modifications for each unit test variation is a clean, maintainable pattern that reduces duplication while providing comprehensive test coverage.tests/testing_data/storageville.sql (1)
1-90: LGTM! Idempotent data population improves test reliability.The conversion to
REPLACE INTOstatements separates schema from data, making the test data idempotent and aligning with the PR's single-source schema approach. The_build_test_dbhelper intests/conftest.pyloads the central schema first fromdata_files/temoa_schema_v4.sql, then executes data scripts, ensuring this data populates correctly. All 27 tables referenced in storageville.sql are defined in the schema.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/testing_configs/config_survival_curve.toml (1)
3-9: Legacy keys still present despite AI summary indicating removal.The AI summary states these legacy configuration keys were removed, but they remain in the code. This issue was previously flagged and remains unresolved.
tests/testing_data/mediumville.sql (1)
102-102: Inadequate inline comment for negative emission coefficient.The comment "needs to be negative as a driver of linked tech...don't ask" remains uninformative. As noted in the previous review, this should explain that: (1)
emission_activitystores emission activity coefficients, (2) negative coefficients represent emissions removal/capture, and (3) this is required by the linked tech constraint.tests/conftest.py (1)
52-68: Re-enable foreign key constraints before commit.Foreign key constraints are disabled on lines 52 and 56 but never re-enabled. While this allows flexible data loading, test databases should still validate referential integrity. Re-enabling constraints before the final commit would catch data integrity issues early.
Based on past review comments, consider applying this fix:
🔎 Proposed fix
# 3. Apply modifications if modifications: for sql, params in modifications: con.execute(sql, params) + # Re-enable foreign key constraints to validate integrity + con.execute('PRAGMA foreign_keys = ON') con.commit()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
tests/conftest.py(2 hunks)tests/testing_configs/config_annualised_demand.toml(1 hunks)tests/testing_configs/config_emissions.toml(1 hunks)tests/testing_configs/config_link_test.toml(1 hunks)tests/testing_configs/config_materials.toml(1 hunks)tests/testing_configs/config_mediumville.toml(1 hunks)tests/testing_configs/config_seasonal_storage.toml(1 hunks)tests/testing_configs/config_storageville.toml(1 hunks)tests/testing_configs/config_survival_curve.toml(1 hunks)tests/testing_configs/config_test_system.toml(1 hunks)tests/testing_configs/config_utopia.toml(1 hunks)tests/testing_configs/config_utopia_gv.toml(1 hunks)tests/testing_configs/config_utopia_myopic.toml(1 hunks)tests/testing_data/annualised_demand.sql(1 hunks)tests/testing_data/emissions.sql(1 hunks)tests/testing_data/mediumville.sql(1 hunks)tests/testing_data/seasonal_storage.sql(1 hunks)tests/testing_data/simple_linked_tech.sql(1 hunks)tests/testing_data/storageville.sql(1 hunks)tests/testing_data/survival_curve.sql(1 hunks)tests/testing_data/utopia_data.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T22:55:02.519Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 186
File: temoa/_internal/temoa_sequencer.py:60-63
Timestamp: 2025-11-03T22:55:02.519Z
Learning: In `TemoaSequencer.__init__`, when a `mode_override` is provided, it must be written back to `self.config.scenario_mode` (not just stored in `self.temoa_mode`) because `HybridLoader.create_data_dict()` has strict consistency checks that validate `config.scenario_mode` matches the presence/absence of `myopic_index`. Without this synchronization, MYOPIC mode would fail with a RuntimeError when the loader is instantiated.
Applied to files:
tests/testing_configs/config_link_test.tomltests/testing_configs/config_storageville.tomltests/testing_configs/config_mediumville.toml
🔇 Additional comments (43)
tests/testing_configs/config_emissions.toml (3)
3-9: Inconsistency between AI summary and code.The AI summary states that
input_database,output_database,neos,solver_name,save_excel,save_duals, andsave_lp_filewere removed from this file. However, these keys still appear in the provided code (lines 3-9) and are not marked as changed. Please verify whether these keys should remain or be removed as part of this refactoring.
16-16: LGTM! Comment cleanup simplifies the configuration.Removing the inline explanatory comments from
weightandmyopic_viewsimplifies the configuration file. This cleanup is consistent with the standardization effort across test configurations.Also applies to: 19-19
10-11: The configuration keystime_sequencingandreserve_marginare properly supported by the codebase. Both are defined intemoa/core/config.py, loaded through the hybrid loader, and have explicit validation logic. The values "seasonal_timeslices" (fortime_sequencing) and "static" (forreserve_margin) are supported in their respective match statements incomponents/time.pyandcomponents/reserves.py. The standardization of these keys across test configurations aligns with the existing implementation.tests/testing_configs/config_mediumville.toml (2)
16-16: LGTM: Comment cleanup.Simplifying the
weightparameter declaration by removing inline comments improves readability without changing functionality.
19-19: LGTM: Comment cleanup.Simplifying the
myopic_viewparameter declaration by removing trailing comments improves consistency with the rest of the file.tests/testing_configs/config_materials.toml (2)
3-9: Inconsistency: Configuration keys not removed as stated.The AI summary indicates that
input_database,output_database,neos,solver_name,save_excel,save_duals, andsave_lp_filewere removed from this file (consistent with removal from other configs). However, these keys are still present in the code. If these are legacy keys that should be removed as part of the consolidation effort, they need to be deleted.Please verify whether these keys should be removed to align with the broader config standardization mentioned in the PR.
10-11: Both configuration keys are valid and properly supported.The additions of
time_sequencing = "seasonal_timeslices"andreserve_margin = "static"are supported configuration parameters. These keys are defined intemoa/core/config.py, integrated into the model components, and the specified values are explicitly handled in the time sequencing and reserve margin calculation logic intemoa/components/time.pyandtemoa/components/reserves.py, respectively. Both are part of the core simulation configuration and align with the standardization effort.tests/testing_configs/config_survival_curve.toml (3)
14-18: LGTM: MGA section properly condensed.The MGA parameter formatting has been condensed while preserving all values. The removal of inline comments improves readability.
21-21: LGTM: Myopic section properly condensed.The inline comment has been removed while preserving the
myopic_viewvalue.
10-11: No changes required. The configuration keystime_sequencingandreserve_marginare valid Temoa parameters with standardized values that are used consistently across all test configs.tests/testing_configs/config_link_test.toml (3)
19-19: LGTM!The standardization of the weight declaration by removing inline comments improves readability.
21-23: No action required: [myopic] section may be present in perfect_foresight configs.The [myopic] section is included in all test configurations as standard practice and is simply unused when
scenario_mode = "perfect_foresight". The validation inHybridLoader.create_data_dict()only enforces consistency betweenscenario_modeand themyopic_indexparameter passed at runtime. Having the configuration section present does not cause errors or unexpected behavior.
13-14: Configuration placement is correct; no issues found.The top-level placement of
time_sequencingandreserve_marginis correct and properly handled by the configuration parser. These keys are defined as root-level parameters intemoa/core/config.py(lines 50-51, 127-128) and correctly accessed bytemoa/data_io/hybrid_loader.pyasself.config.time_sequencingandself.config.reserve_margin(lines 265-267). The TOML file correctly places these keys before any table headers, storing them in the root table as expected.tests/testing_configs/config_utopia_myopic.toml (4)
3-9: Inconsistency: AI summary claims these options were removed, but they remain in the file.The AI summary states that
input_database,output_database,neos,solver_name,save_excel,save_duals, andsave_lp_filewere removed, yet they're still present in the configuration file.
16-16: LGTM: Formatting standardization.The formatting change to the
weightparameter aligns with the configuration cleanup effort described in the PR objectives.
19-20: Myopic parameters are correctly configured.The
view_depth = 2andstep_size = 1parameters are valid for myopic mode. Both are required integers that control the sequencing of optimization iterations, where view_depth specifies the number of periods analyzed per iteration and step_size determines the step length. The configuration meets the constraint that step_size must not exceed view_depth, consistent with other test configurations in the repository.
10-11: Verify the new configuration parameters are valid and appropriately set.These parameters appear to be newly standardized across test configurations. Please confirm that:
- The parameter names (
time_sequencing,reserve_margin) are recognized by the application- The values (
"seasonal_timeslices","static") are valid options- These specific values are appropriate for the myopic Utopia test scenario
tests/testing_configs/config_test_system.toml (3)
3-9: Inconsistency with AI summary.The AI-generated summary states that these legacy configuration keys (input_database, output_database, neos, solver_name, save_excel, save_duals, save_lp_file) were removed, but they are still present in the file. Please verify whether these keys should remain or if the removal was incomplete.
16-16: LGTM!The formatting changes (removal of trailing comments) on these lines are clean and do not affect the configuration values.
Also applies to: 19-19
10-11: No action required. The top-level configuration keystime_sequencingandreserve_marginare properly recognized and handled by the configuration parser, model components, and validators. The values specified for testing ("seasonal_timeslices" and "static") are valid and consistent with usage patterns throughout the codebase.tests/testing_configs/config_utopia.toml (4)
3-9: Inconsistency between AI summary and code.The AI summary claims these configuration options were removed, but they're still present in the file. The code shows these lines as unchanged (not marked with
~), indicating they remain in the configuration.
21-21: LGTM!The myopic configuration value remains unchanged; this is just a formatting cleanup (removed trailing comment).
14-18: All MGA configuration parameters are correctly configured. The parameter names match the expected schema, the value ranges are valid (with test-appropriate values stricter than production defaults), and both "tech_category_activity" and "hull_expansion" are supported enum options that work together in the codebase.
10-11: Configuration options are correctly added and standardized.The additions of
time_sequencing = "seasonal_timeslices"andreserve_margin = "static"are consistent with the codebase:
- Both are recognized configuration parameters in the application
- "seasonal_timeslices" and "static" are valid values for their respective options
- Both are correctly placed at the root level, matching the pattern across all other test configuration files
tests/testing_configs/config_storageville.toml (3)
16-16: LGTM: Comment cleanup.The removal of trailing inline comments from
weightandmyopic_viewparameters improves consistency and reduces clutter. Ensure that any removed documentation is captured elsewhere if needed (e.g., in main documentation or schema definitions).Also applies to: 19-19
10-11: Parameterstime_sequencingandreserve_marginare valid top-level configuration options with recognized values. The codebase confirms"seasonal_timeslices"and"static"as accepted values, and these parameters are properly loaded and used throughout the system.
2-2: No changes needed. The configuration is valid as-is.The
[myopic]section in test configuration files withscenario_mode = "perfect_foresight"is intentional and causes no issues. The myopic section is parsed intoconfig.myopic_inputsbut is only used whenscenario_mode = "MYOPIC". Since the execution mode is perfect foresight, the myopic settings are safely ignored. This pattern is consistently used across all test configurations in the repository.tests/testing_configs/config_annualised_demand.toml (2)
10-11: LGTM - Config standardization.Adding top-level
time_sequencingandreserve_marginkeys standardizes the configuration format across test configs.
14-21: LGTM - Cosmetic cleanup.Removing inline comments improves readability while preserving semantic values.
tests/testing_configs/config_utopia_gv.toml (2)
10-11: LGTM - Config standardization.Top-level
time_sequencingandreserve_marginkeys align with the standardization effort across test configurations.
14-22: LGTM - Inline comment cleanup.Preserves all semantic values while improving readability.
tests/testing_configs/config_seasonal_storage.toml (2)
10-11: LGTM - Configuration standardization.Top-level configuration keys align with the broader standardization effort.
14-21: LGTM - Formatting improvements.Semantic values preserved with improved readability.
tests/testing_data/annualised_demand.sql (1)
1-55: Verify foreign key constraint settings and schema loading order in test infrastructure.REPLACE INTO performs DELETE + INSERT when primary key conflicts occur. With ON DELETE CASCADE constraints, this can trigger automatic deletion of matching rows in child tables. However, SQLite disables foreign key constraints by default, so this risk only materializes if the test infrastructure explicitly enables them via PRAGMA. Confirm: (1) whether PRAGMA foreign_keys is enabled in the test database setup, (2) the schema loading order ensures the central schema is created before annualised_demand.sql executes, and (3) no cascade rules are defined that could cause unintended deletions.
tests/testing_data/mediumville.sql (1)
1-198: Schema dependency is already satisfied by the test infrastructure. The_build_test_db()function in conftest.py explicitly loads the central schema (temoa_schema_v4.sql) before executing any data scripts, including mediumville.sql. No action required.Likely an incorrect or invalid review comment.
tests/testing_data/storageville.sql (1)
1-90: Schema loading sequence is properly managed in test infrastructure.The test infrastructure correctly handles the data-only approach through the
_build_test_db()function, which enforces a three-step sequence: (1) load central schema fromtemoa_schema_v4.sql, (2) load data scripts likestorageville.sql, (3) apply modifications. This dependency is properly sequenced and will not fail due to table definition order.tests/testing_data/survival_curve.sql (1)
1-185: No issues found. The test framework correctly establishes schema loading precedence:_build_test_db()loadstemoa_schema_v4.sql(containing all CREATE TABLE statements for commodity, cost_fixed, demand, etc.) before executingsurvival_curve.sql(containing the REPLACE INTO operations). The dependency is properly managed.tests/testing_data/seasonal_storage.sql (1)
1-111: The test infrastructure properly loads the central schema before executing data-only SQL files. The_build_test_db()function inconftest.pyensures schema initialization occurs first, followed by data script execution. Foreign key constraints are also disabled during database construction, eliminating potential constraint violations from REPLACE operations. No action required.Likely an incorrect or invalid review comment.
tests/testing_data/simple_linked_tech.sql (1)
1-63: LGTM! Clean conversion to idempotent upserts.The conversion from transactional INSERTs to REPLACE INTO statements makes test data loading idempotent and robust. This aligns well with the centralized schema-first approach introduced in conftest.py.
tests/testing_data/emissions.sql (1)
1-104: LGTM! Consistent idempotent data seeding.The file follows the same REPLACE INTO pattern as other refactored test data files, ensuring idempotent test database construction.
tests/conftest.py (3)
36-39: Excellent centralization of test paths.The introduction of centralized path constants (TEST_DATA_PATH, TEST_OUTPUT_PATH, SCHEMA_PATH) improves maintainability and makes the test infrastructure easier to understand.
97-168: Excellent refactoring of unit test database generation.The function now cleanly generates multiple test database variations from a single source (utopia_data.sql) using targeted SQL modifications. This approach is much more maintainable than maintaining separate full database files for each test case.
76-95: Bothutopia.sqliteandmyo_utopia.sqliteintentionally use the same source data—this is correct.The myopic optimization mode is a solving strategy (sequential planning over multiple periods), not a data characteristic. Both databases should contain identical test data because the test data represents the same system regardless of whether it's solved with perfect foresight or myopic optimization. The separate database file exists for test isolation and clarity, not because the underlying data should differ. The current implementation correctly tests that the myopic solver handles the standard test data appropriately.
ParticularlyPythonicBS
force-pushed
the
ref/consolidating_testing_dbs
branch
from
71e9fea to
ca7f089
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/testing_configs/config_survival_curve.toml (1)
3-9: Legacy keys still present in the file.The previous review flagged that these legacy keys (
input_database,output_database,neos,solver_name,save_excel,save_duals,save_lp_file) should be removed as part of the config standardization. The fix was marked as addressed, but these keys still appear in the current file state.Please verify whether the fix from commit 71e9fea was reverted or if there's a different expectation for this specific config file:
#!/bin/bash # Check git history for this file to see if legacy keys were removed and re-added git log --oneline -10 -- tests/testing_configs/config_survival_curve.toml # Check current file content for legacy keys cat tests/testing_configs/config_survival_curve.toml | head -15
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (22)
temoa/tutorial_assets/utopia.sql(1 hunks)tests/conftest.py(2 hunks)tests/testing_configs/config_annualised_demand.toml(1 hunks)tests/testing_configs/config_emissions.toml(1 hunks)tests/testing_configs/config_link_test.toml(1 hunks)tests/testing_configs/config_materials.toml(1 hunks)tests/testing_configs/config_mediumville.toml(1 hunks)tests/testing_configs/config_seasonal_storage.toml(1 hunks)tests/testing_configs/config_storageville.toml(1 hunks)tests/testing_configs/config_survival_curve.toml(1 hunks)tests/testing_configs/config_test_system.toml(1 hunks)tests/testing_configs/config_utopia.toml(1 hunks)tests/testing_configs/config_utopia_gv.toml(1 hunks)tests/testing_configs/config_utopia_myopic.toml(1 hunks)tests/testing_data/annualised_demand.sql(1 hunks)tests/testing_data/emissions.sql(1 hunks)tests/testing_data/mediumville.sql(1 hunks)tests/testing_data/seasonal_storage.sql(1 hunks)tests/testing_data/simple_linked_tech.sql(1 hunks)tests/testing_data/storageville.sql(1 hunks)tests/testing_data/survival_curve.sql(1 hunks)tests/testing_data/utopia_data.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T22:55:02.519Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 186
File: temoa/_internal/temoa_sequencer.py:60-63
Timestamp: 2025-11-03T22:55:02.519Z
Learning: In `TemoaSequencer.__init__`, when a `mode_override` is provided, it must be written back to `self.config.scenario_mode` (not just stored in `self.temoa_mode`) because `HybridLoader.create_data_dict()` has strict consistency checks that validate `config.scenario_mode` matches the presence/absence of `myopic_index`. Without this synchronization, MYOPIC mode would fail with a RuntimeError when the loader is instantiated.
Applied to files:
tests/testing_configs/config_mediumville.tomltests/testing_configs/config_link_test.tomltests/testing_configs/config_storageville.toml
⏰ 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). (4)
- GitHub Check: setup and test (windows-latest, 3.12)
- 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.13)
🔇 Additional comments (33)
tests/testing_configs/config_emissions.toml (4)
3-9:⚠️ Summary inconsistency: claimed removals are still present in the code.The AI summary states that
input_database,output_database,neos,solver_name,save_excel,save_duals, andsave_lp_filewere removed, but these lines appear unchanged (unmarked with~) in the provided code. This contradicts the summary's claim and the broader PR pattern mentioned for other config files.Given the PR objective to consolidate test database creation via a central schema approach, these legacy top-level database path declarations may conflict with the new centralized design.
Clarify: Were lines 3–9 intentionally preserved in this file while being removed from other configs, or should they also be removed for consistency across the test suite?
10-11: Added time_sequencing and reserve_margin correctly.The new top-level keys
time_sequencing = "seasonal_timeslices"andreserve_margin = "static"align with the summary and the standardization pattern described for other config files in the PR.
16-16: MGA weight configuration simplified appropriately.The change from an inline-commented form to
weight = "integer"is cleaner and matches the standardization pattern noted in the summary for other config files.Confirm that
"integer"is a valid and supported value for the MGA weight parameter in the Temoa codebase.
19-19: Comment removed, value preserved correctly.The myopic_view value is unchanged; only the inline comment has been removed, which aligns with the documentation cleanup described in the PR.
tests/testing_configs/config_utopia_gv.toml (3)
14-19: LGTM: MGA section cleanup.The consolidation of the MGA parameter block by removing inline comments improves readability while preserving the parameter values. The configuration is clean and standardized.
21-22: LGTM: myopic section cleanup.The removal of the end-of-line comment for
myopic_viewaligns with the overall cleanup effort while preserving the configuration value.
1-12: Config parameters are valid.Both
time_sequencing = "seasonal_timeslices"andreserve_margin = "static"are properly defined top-level configuration options in the TOML schema, as confirmed by their presence intemoa/core/config.pyand integration throughout the model. The configuration aligns with all other test configs in the repository.tests/testing_configs/config_annualised_demand.toml (3)
13-18: LGTM!The MGA section configuration values are well-formed and reasonable for test purposes. The parameters are properly formatted and within expected ranges.
20-21: LGTM!The myopic section configuration is properly formatted with a reasonable value.
3-11: Inconsistency between summary and actual code: Both old and new configuration options coexist.The summary describes old options (input_database, output_database, neos, solver_name, save_excel, save_duals, save_lp_file) as "replaced" by new ones (time_sequencing, reserve_margin). However, analysis across all test config files shows both sets present in every configuration—this is a systematic pattern, not incomplete refactoring.
Clarify whether:
- Both sets are intentionally retained for backward compatibility or phased deprecation, or
- Old options should be removed as part of the refactor.
temoa/tutorial_assets/utopia.sql (1)
461-461: LGTM: Improved numeric readability.The conversion from scientific notation (5.669999999999999574e+01) to decimal (56.7) improves readability while maintaining mathematical equivalence.
tests/testing_configs/config_mediumville.toml (1)
10-11: New configuration keys added.The
time_sequencingandreserve_marginkeys follow the same pattern as other config files in this PR. Configuration changes appear consistent across test configs.tests/testing_configs/config_test_system.toml (1)
10-11: LGTM: Consistent configuration standardization.The addition of
time_sequencingandreserve_marginis consistent with the broader refactoring across all test configuration files.tests/testing_configs/config_utopia.toml (2)
14-18: Verify MGA parameter changes align with test expectations.The MGA section has been significantly refactored with new parameter names and values:
cost_epsilon = 0.03(wasslack = 0.1in other configs)iteration_limit = 15(wasiterations = 4in other configs)- New:
time_limit_hrs = 1- New:
axis = "tech_category_activity"- New:
weighting = "hull_expansion"(wasweight = "integer"in other configs)These changes suggest an MGA API update. Confirm that test expectations and assertions are updated to match the new parameter semantics.
#!/bin/bash # Description: Check MGA parameter usage and test expectations # Find MGA-related test code rg -n "MGA|mga" tests/ --type py -A 3 -B 3 # Check for uses of old parameter names rg -n "slack|iterations[^_]|weight" tests/testing_configs/ # Check MGA implementation for parameter handling rg -n "cost_epsilon|iteration_limit|time_limit_hrs|hull_expansion" temoa/ --type py
10-11: Both configuration keys are properly supported and integrated. Thetime_sequencingparameter (with value "seasonal_timeslices") andreserve_marginparameter (with value "static") are defined in temoa/core/config.py, loaded via temoa/data_io/hybrid_loader.py, and have complete downstream implementations in temoa/components/time.py and temoa/components/reserves.py respectively. Both include validation and proper error handling for unsupported values.tests/testing_data/emissions.sql (1)
1-104: Schema initialization is correctly handled; no changes needed.The test database setup in
conftest.pyproperly loads the central schema (temoa_schema_v4.sql) before executing data scripts likeemissions.sql. The_build_test_db()function executes the schema first (line 50), then loads data scripts (lines 54-56) with foreign keys disabled, ensuringREPLACE INTOstatements work without constraint violations. The explicit value counts in theREPLACE INTOstatements align with the schema table definitions.tests/testing_configs/config_storageville.toml (1)
10-11: LGTM! Configuration parameters standardized.The addition of explicit
time_sequencingandreserve_marginparameters at the top level improves clarity and aligns with the PR's broader config standardization effort.tests/testing_data/utopia_data.sql (1)
1-559: LGTM! Central Utopia seed data is well-structured.The file successfully serves as a single source of truth for Utopia test data with consistent idempotent REPLACE INTO statements. The previous floating-point notation issue at Line 311 has been addressed.
tests/testing_data/mediumville.sql (1)
102-102: LGTM! Negative emission coefficient is now well-documented.The updated comment clearly explains that
emission_activitystores emission activity coefficients (not efficiencies), that negative values represent CO2 removal/capture, and the relationship to the linked-tech constraint. This addresses the previous review feedback.tests/testing_configs/config_materials.toml (1)
10-11: LGTM! Configuration standardization applied.The addition of explicit
time_sequencingandreserve_marginparameters is consistent with the PR's config refactoring pattern.tests/testing_configs/config_seasonal_storage.toml (1)
10-11: LGTM! Config parameters standardized.The explicit top-level
time_sequencingandreserve_marginparameters improve config clarity and consistency across test files.tests/conftest.py (4)
36-40: LGTM! Centralized test data paths improve maintainability.The introduction of
TEST_DATA_PATH,TEST_OUTPUT_PATH, andSCHEMA_PATHconstants provides a clear, centralized location for test infrastructure paths.
42-71: LGTM! Robust test DB builder with proper constraint handling.The
_build_test_dbhelper provides a clean, reusable pattern for test database construction. The previous review concerns have been addressed:
- Type annotation correctly uses
| Noneunion syntax (line 45)- Foreign key constraints are properly re-enabled before commit (lines 69-70)
The function correctly:
- Disables foreign keys during data loading for flexible insertion order
- Loads the central schema
- Applies data scripts and modifications
- Re-enables foreign key constraints before committing
73-98: LGTM! Streamlined database refresh using centralized builder.The refactored
refresh_databasesfunction leverages the new_build_test_dbhelper to eliminate duplication and provide a consistent database creation pattern across all test databases.
100-170: LGTM! Unit test DB creation centralized from single source.The refactored
create_unit_test_dbsfunction successfully centralizes unit test database generation fromutopia_data.sqlas a single source of truth, applying targeted modifications for each test variation. This approach improves maintainability and reduces duplication.tests/testing_data/storageville.sql (1)
6-6: Correct line number for commodity type difference in seasonal_storage.sql.Line 6 in
storageville.sqldefines theearthcommodity with type'p'(physical commodity), but inseasonal_storage.sqlline 12 (not line 13), the same commodity is defined with type's'(source commodity). Verify this difference is intentional for the specific test scenarios, as both definitions appear to coexist across test files.tests/testing_configs/config_survival_curve.toml (2)
10-11: LGTM!The new
time_sequencingandreserve_marginkeys align with the PR's standardization of test config files.
14-21: LGTM!The MGA and myopic sections are properly formatted with the expected parameter values.
tests/testing_configs/config_utopia_myopic.toml (3)
10-11: LGTM!The new
time_sequencingandreserve_marginkeys are correctly added, consistent with the PR's standardization approach.
16-20: LGTM!The reformatted
weightparameter and new myopic parameters (view_depth,step_size) are properly configured.
3-9: No action needed—these keys are standard across all config files.The keys in lines 3-9 (
input_database,output_database,neos,solver_name,save_excel,save_duals,save_lp_file) are present in all 12 test configuration files, not justconfig_utopia_myopic.toml. This is the standardized structure—they are required configuration parameters, not legacy keys to be removed.Likely an incorrect or invalid review comment.
tests/testing_configs/config_link_test.toml (2)
13-14: Parameters are correctly configured at root level.The
time_sequencingandreserve_marginparameters at root level are properly handled by the configuration loader. The codebase confirms thattemoa/core/config.pyexpects these as root-level attributes, and all test configuration files consistently place them at this level.
21-23: The configuration is valid and intentional. The[myopic]section is silently ignored whenscenario_mode = "perfect_foresight"becauseHybridLoader.create_data_dict()is called withmyopic_index=Nonein perfect foresight mode. The consistency checks only enforce errors whenmyopic_indexis actually provided to the loader—not when it's absent. Having an unused[myopic]section in a perfect_foresight config does not cause runtime errors.Likely an incorrect or invalid review comment.
| slack = 0.1 | ||
| iterations = 4 | ||
| weight = "integer" # currently supported: [integer, normalized] | ||
| weight = "integer" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for MGA weight usage in Python files
echo "=== Searching for MGA weight usage in Python code ==="
rg -n -C3 'weight.*MGA|MGA.*weight' --type py
echo ""
echo "=== Checking config_link_test.toml content ==="
cat tests/testing_configs/config_link_test.toml
echo ""
echo "=== Checking other TOML files in testing_configs ==="
fd -e toml . tests/testing_configs/ -exec basename {} \;
echo ""
echo "=== Comparing weight values across all TOML configs ==="
fd -e toml . tests/testing_configs/ -exec grep -H "weight" {} \;Repository: TemoaProject/temoa
Length of output: 4513
🏁 Script executed:
#!/bin/bash
# Search for MgaWeighting enum definition
echo "=== Searching for MgaWeighting enum ==="
rg -n "class MgaWeighting|enum MgaWeighting" --type py
echo ""
echo "=== Searching for config parsing of MGA section ==="
rg -n -A5 "mga_inputs|MGA\]" --type py temoa/core/config.py | head -50
echo ""
echo "=== Searching for weight/weighting parameter handling ==="
rg -n "weight|weighting" temoa/extensions/modeling_to_generate_alternatives/ --type pyRepository: TemoaProject/temoa
Length of output: 4125
🏁 Script executed:
#!/bin/bash
# Get the MgaWeighting enum definition
echo "=== MgaWeighting enum definition ==="
cat temoa/extensions/modeling_to_generate_alternatives/mga_constants.py
echo ""
echo "=== How [MGA] config section is parsed from TOML ==="
rg -n -B5 -A10 "def.*read_config|toml.load" --type py temoa/core/config.py | head -80Repository: TemoaProject/temoa
Length of output: 1460
🏁 Script executed:
#!/bin/bash
# Search for where TOML config file is read
echo "=== Finding TOML config file reading ==="
rg -n "toml.load|ConfigParser|get.*MGA|\[MGA\]" --type py temoa/core/ | head -40
echo ""
echo "=== Checking temoa_model.py for config parsing ==="
rg -n -B3 -A10 "def parse_config|MGA" temoa_model.py 2>/dev/null | head -80Repository: TemoaProject/temoa
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Search for toml library usage
echo "=== Searching for toml import and usage ==="
rg -n "import toml|from toml" --type py
echo ""
echo "=== Search for where config file argument is processed ==="
rg -n -B3 -A10 "argparse|config.*file|--config" --type py | head -100
echo ""
echo "=== Look at main entry point ==="
head -100 temoa_model.pyRepository: TemoaProject/temoa
Length of output: 6898
🏁 Script executed:
#!/bin/bash
# Find the build_config method
echo "=== Finding TemoaConfig.build_config method ==="
rg -n -A40 "def build_config" temoa/core/config.py
echo ""
echo "=== Look for where MGA section is parsed ==="
rg -n -B5 -A15 "tomllib.load|MGA.*=|weights" temoa/core/config.py | head -100Repository: TemoaProject/temoa
Length of output: 4326
🏁 Script executed:
#!/bin/bash
# Search for any MGA config usage in other files or documentation
echo "=== Check all TOML files for MGA section ==="
rg -n "\[MGA\]" --type toml
echo ""
echo "=== Check documentation or comments about MGA config ==="
rg -n "weight|weighting" temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py -B2 -A2Repository: TemoaProject/temoa
Length of output: 2184
🏁 Script executed:
#!/bin/bash
# Check what weight/weighting values other config files use
echo "=== Check weight/weighting values in all MGA sections ==="
rg -A5 "\[MGA\]" --type toml tests/testing_configs/ | grep -E "\[MGA\]|weight"
echo ""
echo "=== Check MgaWeighting enum valid values ==="
rg -A5 "class MgaWeighting" temoa/extensions/modeling_to_generate_alternatives/mga_constants.pyRepository: TemoaProject/temoa
Length of output: 1697
Change MGA weighting parameter name and use a valid enum value.
The MGA configuration uses weight = "integer", but the code expects a parameter named weighting with a value matching the MgaWeighting enum. Valid values are "hull_expansion" or "unused_stuff" (case-insensitive). Refer to config_utopia.toml or config_annualised_demand.toml for the correct format: weighting = "hull_expansion".
🤖 Prompt for AI Agents
In tests/testing_configs/config_link_test.toml around line 19, the MGA config
uses weight = "integer" but the code expects a parameter named weighting with a
MgaWeighting enum value; change the key to weighting and set a valid enum string
(case-insensitive) such as "hull_expansion" or "unused_stuff" — e.g. replace the
line with weighting = "hull_expansion".
| REPLACE INTO "season_label" VALUES('apr_w1','sequential season - day in first week of September'); | ||
| REPLACE INTO "season_label" VALUES('apr_w2','sequential season - day in second week of September'); | ||
| REPLACE INTO "season_label" VALUES('apr_w3','sequential season - day in third week of September'); | ||
| REPLACE INTO "season_label" VALUES('apr_w4','sequential season - day in fourth week of September'); |
There was a problem hiding this comment.
Fix copy-paste error in season label descriptions.
Lines 64-67 define April weeks (apr_w1 through apr_w4) but the descriptions incorrectly state "September" instead of "April".
🔎 Proposed fix
-REPLACE INTO "season_label" VALUES('apr_w1','sequential season - day in first week of September');
-REPLACE INTO "season_label" VALUES('apr_w2','sequential season - day in second week of September');
-REPLACE INTO "season_label" VALUES('apr_w3','sequential season - day in third week of September');
-REPLACE INTO "season_label" VALUES('apr_w4','sequential season - day in fourth week of September');
+REPLACE INTO "season_label" VALUES('apr_w1','sequential season - day in first week of April');
+REPLACE INTO "season_label" VALUES('apr_w2','sequential season - day in second week of April');
+REPLACE INTO "season_label" VALUES('apr_w3','sequential season - day in third week of April');
+REPLACE INTO "season_label" VALUES('apr_w4','sequential season - day in fourth week of April');📝 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.
| REPLACE INTO "season_label" VALUES('apr_w1','sequential season - day in first week of September'); | |
| REPLACE INTO "season_label" VALUES('apr_w2','sequential season - day in second week of September'); | |
| REPLACE INTO "season_label" VALUES('apr_w3','sequential season - day in third week of September'); | |
| REPLACE INTO "season_label" VALUES('apr_w4','sequential season - day in fourth week of September'); | |
| REPLACE INTO "season_label" VALUES('apr_w1','sequential season - day in first week of April'); | |
| REPLACE INTO "season_label" VALUES('apr_w2','sequential season - day in second week of April'); | |
| REPLACE INTO "season_label" VALUES('apr_w3','sequential season - day in third week of April'); | |
| REPLACE INTO "season_label" VALUES('apr_w4','sequential season - day in fourth week of April'); |
🤖 Prompt for AI Agents
In tests/testing_data/seasonal_storage.sql around lines 64 to 67, the
season_label entries for 'apr_w1'...'apr_w4' incorrectly mention "September" in
their descriptions; update those descriptions to reference "April" (e.g.,
"sequential season - day in first week of April", etc.) so labels match the
April keys.
Addresses #235
Refactored testing database generation to first load a central schema and then load data only SQL files
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.