Add branch-level outputs to PowerFlow predict_step - #122
Conversation
Signed-off-by: Naomi Simumba <7224231+naomi-simumba@users.noreply.github.com>
Signed-off-by: Naomi Simumba <7224231+naomi-simumba@users.noreply.github.com>
|
@naomi-simumba Thanks for this — a clean, well-scoped addition, and the dedicated test file (structure, numerical properties, and correctness against real case14 data) is exactly the kind of coverage we like to see. All CI checks are green, commits are signed off (DCO passing), and the code correctly reuses What's needed (minor):
Everything else looks in good shape from a pre-review standpoint — no new params or dependencies to wire into the config YAMLs or |
|
Looks good to me. README and docs/quick_start/quick_start.md still only mention <grid_name>_predictions.parquet. Please add <grid_name>_branch_predictions.parquet next to the --save_output / --output_path notes so users can find the new table. |
|
@albanpuech agreed — that's the one remaining item on this PR, and I can point @naomi-simumba at the exact spots:
Each currently documents only Nothing else outstanding from the pre-review: CI green, DCO signed off, no new params or deps to wire into the config YAMLs or |
|
@naomi-simumba The new That was the only outstanding point from the pre-review. CI is currently re-running on the new commit; assuming it comes back green (DCO already passes), this looks ready for a maintainer's final look. Deferring the merge decision to a human maintainer. — 🤖 _automated pre-review; a maintainer will follow up_ |
albanpuech
left a comment
There was a problem hiding this comment.
Optional: predict_step already runs ComputeBranchFlow for residuals. This helper runs it again on the same eval_bus. You could pass those Pft/Qft in instead of recomputing. Up to you.
Co-authored-by: PUECH Alban <72336171+albanpuech@users.noreply.github.com> Signed-off-by: naomi-simumba <7224231+naomi-simumba@users.noreply.github.com>
|
@naomi-simumba The new docstring commit (9036725) is a nice clarification — spelling out that One small heads-up: the three new blank lines inside the docstring were added with trailing whitespace ( |
|
@naomi-simumba The new CI is re-running on the new commit ( |
romeokienzler
left a comment
There was a problem hiding this comment.
All CI checks green, DCO signed off, docs updated (both README and quick_start) to document _branch_predictions.parquet. The outstanding doc gap raised by @albanpuech and in prior pre-reviews is resolved. The optional refactor note (avoid recomputing branch flows) was marked non-blocking. Code is clean, tests are thorough. Approving.
Summary
Closes [https://github.com//issues/96]. Extends the PowerFlow task's predict_step to return branch-level predictions alongside bus-level predictions, and wires them through the existing CLI save logic.
Changes
gridfm_graphkit/tasks/utils.py: Addedcompute_branch_predictions()— computes active/reactive branch flows (Pft, Qft), thermal excess, angle differences, and angle bound violations for both predicted and ground-truth bus statesgridfm_graphkit/tasks/pf_task.py: Imported compute_branch_predictions from tasks.utils. Calledcompute_branch_predictionsinpredict_stepand added "branch" to the returned dictgridfm_graphkit/cli.py: Added an explicit "branch" case to_prediction_output_filenameso branch predictions are saved as{grid_name}_branch_predictions.parquettests/test_compute_branch_predictions.py: Unit and integration tests for compute_branch_predictions covering output structure, numerical properties, and correctness against the case14_ieee reference data