Remove duplicate matrix-based NR implementation from test suite#370
Merged
Conversation
Closed
Agent-Logs-Url: https://github.com/Sienna-Platform/PowerFlows.jl/sessions/d6ca2f46-ff83-44a5-9c25-3679f1e99f81 Co-authored-by: jd-lara <16385323+jd-lara@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor testing to remove inefficient NR implementation
Remove duplicate matrix-based NR implementation from test suite
May 16, 2026
Contributor
Performance ResultsPrecompile Time
Solve Time
|
Agent-Logs-Url: https://github.com/Sienna-Platform/PowerFlows.jl/sessions/e234fc62-7d8f-4670-b359-148fb2607001 Co-authored-by: jd-lara <16385323+jd-lara@users.noreply.github.com>
Contributor
Author
Done in cb98086. Restored cross-solver validation using
All 11 affected tests pass. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
jd-lara
approved these changes
May 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The test suite carried a second, matrix-based Newton-Raphson implementation (
LUACPowerFlowintest/test_utils/legacy_pf.jl) used only as a benchmark, forcing maintenance of two NR code bases.Changes
Deleted
test/test_utils/legacy_pf.jl(244 lines:LUACPowerFlow,_legacy_dSbus_dV,_legacy_J, and the duplicate_newton_power_flowmethod).PowerFlowsTests.jl: dropped theincludeand removedLUACPowerFlowfromAC_SOLVERS_TO_TEST.Dropped LU-specific testsets in
test_psse_export.jlandtest_power_flow_data.jl.Restored cross-solver validation using
TrustRegionACPowerFlowin the places whereLUACPowerFlowwas previously used as the comparator:test_multiperiod_ac_power_flow.jl: re-added the "compare results for different solvers" testset, now comparingTrustRegionACPowerFlowvsNewtonRaphsonACPowerFlowon bus magnitudes, angles, and arc active/reactive flows.test_solve_power_flow.jl"Compare larger grid…": renamed to "KLU vs NewtonRaphson vs TrustRegion" and added TrustRegion as a third comparator against the default (KLU) solver on the 2000-bus matpower system.test_solve_power_flow.jl"voltage_stability_factors": restored thevoltage_stability_factorsfield equality check (TrustRegion vs NewtonRaphson) alongside the production-Jacobian / PROPACK SVD reference check.Removed
LUACPowerFlowskip/exclusion filters intest_distributed_slack.jlandtest_lcc_ac_solver(no longer needed).Refactored
voltage_stability_factorstest to source the Jacobian from the production path, eliminating the last consumers of_legacy_dSbus_dV/_legacy_Jwhile preserving the PROPACK SVD reference check:Note on the linked PR #53 review comment
The referenced thread (
r1942073313) was an informational note about gracefully skipping tests that depend onsystem_240[32].jsonwhen the file is absent. The current tree has no remaining references tosystem_240; that concern appears to have been resolved separately.