Ensure all systems are returned in SYSTEM_BASE#186
Conversation
Closes #175 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The failure is an Aqua test, for stale deps. Not sure how to fix or if that's signficant. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
=======================================
Coverage 92.67% 92.67%
=======================================
Files 18 18
Lines 4475 4475
=======================================
Hits 4147 4147
Misses 328 328
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR standardizes the units base returned by build_system by forcing all built/deserialized PowerSystems.System objects to use "SYSTEM_BASE", and adds a regression test for a Matpower case.
Changes:
- Force
"SYSTEM_BASE"in_build_systemfor all systems built or deserialized viabuild_system. - Remove an explicit units-base override from
make_modified_RTS_GMLC_sys(now relying on the centralized behavior). - Add a unit test asserting Matpower systems built through
build_systemreturn"SYSTEM_BASE".
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/test_unit_system.jl | Adds a test asserting build_system(MatpowerTestSystems, ...) returns a system in "SYSTEM_BASE". |
| src/build_system.jl | Centralizes units-base normalization by calling PSY.set_units_base_system!(sys, "SYSTEM_BASE") in _build_system. |
| src/library/psi_library.jl | Removes a per-system units-base override from make_modified_RTS_GMLC_sys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| generator_mapping_file = joinpath(MAP_DIR, "generator_mapping.yaml"), | ||
| ) | ||
|
|
||
| sys = PSY.System(rawsys; time_series_resolution = resolution, sys_kwargs...) |
There was a problem hiding this comment.
make_modified_RTS_GMLC_sys performs several numeric threshold checks later in the function (e.g. get_base_power(g) > 3, get_rating(d) < 3) that appear to assume the system is in SYSTEM_BASE (3 pu == 300 MW on a 100 MVA base). Removing the set_units_base_system! call means those thresholds will be evaluated in whatever units base PSY.System(rawsys; ...) defaults to, which can change which components are modified/removed and produce a different (likely incorrect) “modified” system. Consider restoring PSY.set_units_base_system!(sys, "SYSTEM_BASE") immediately after constructing sys, or otherwise ensure the subsequent edits use a units-base-invariant quantity.
| sys = PSY.System(rawsys; time_series_resolution = resolution, sys_kwargs...) | |
| sys = PSY.System(rawsys; time_series_resolution = resolution, sys_kwargs...) | |
| PSY.set_units_base_system!(sys, "SYSTEM_BASE") |
| if !skip_serialization && isempty(sys_args) | ||
| PSY.to_json(sys, serialized_filepath; force = true) | ||
| #serialize_time = time() - start | ||
| serialize_case_parameters(case_args) | ||
| end | ||
| # set_stats!(sys_descriptor, SystemBuildStats(construct_time, serialize_time)) | ||
| else | ||
| @debug "Deserialize system from file" sys_descriptor.name | ||
| start = time() | ||
| # time_series_in_memory = get(kwargs, :time_series_in_memory, false) | ||
| file_path = get_serialized_filepath(name, case_args) | ||
| sys = PSY.System(file_path; assign_new_uuids = assign_new_uuids, sys_args...) | ||
| PSY.get_runchecks(sys) | ||
| # update_stats!(sys_descriptor, time() - start) | ||
| end | ||
| PSY.set_units_base_system!(sys, "SYSTEM_BASE") | ||
| print_stat ? print_stats(sys_descriptor) : nothing |
There was a problem hiding this comment.
PSY.set_units_base_system! is currently called after the system may have been serialized via PSY.to_json. That means the JSON on disk can represent a different units base than the sys object returned to the caller, which is surprising and can lead to inconsistencies when debugging or when other tooling reads the serialized file. Consider setting the units base immediately after sys is built/loaded (before to_json and before get_runchecks).
| @testset "build_system returns SYSTEM_BASE" begin | ||
| # Matpower systems were previously built in DEVICE_BASE | ||
| sys = build_system(MatpowerTestSystems, "matpower_case5_sys"; force_build = true) | ||
| @test PSY.get_units_base(sys) == "SYSTEM_BASE" |
There was a problem hiding this comment.
This test only exercises the force_build=true path. Since the units-base normalization was added to _build_system after an if/else that also handles deserialization, it would be good to add an assertion for the deserialization path too (e.g., call build_system a second time with force_build=false and re-check the units base).
| @test PSY.get_units_base(sys) == "SYSTEM_BASE" | |
| @test PSY.get_units_base(sys) == "SYSTEM_BASE" | |
| # Also test the deserialization path (force_build = false) | |
| sys2 = build_system(MatpowerTestSystems, "matpower_case5_sys"; force_build = false) | |
| @test PSY.get_units_base(sys2) == "SYSTEM_BASE" |
…ation path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
PSY.set_units_base_system!(sys, "SYSTEM_BASE")in_build_systemso all systems are consistently returned inSYSTEM_BASE, regardless of how the underlying build function constructs themset_units_base_system!call inmake_modified_RTS_GMLC_sysDEVICE_BASE) is nowSYSTEM_BASECloses #175
Test plan
test_unit_systempasses: buildsmatpower_case5_syswithforce_build = trueand assertsSYSTEM_BASE🤖 Generated with Claude Code