Expose empty PowerFactory contingency cases - #83
qian-harvard merged 2 commits into
Conversation
qian-harvard
left a comment
There was a problem hiding this comment.
Both items from #80 are done, and done the way I would have done them. Small, rebased on current main, green on all four interpreters. Verified locally: 32 in the PowerFactory suite, 447 in the full CI set — unchanged from main, which is right, since this adds assertions to an existing test rather than new test functions.
What I checked
The point of the finding was that an agent could not distinguish "nothing is configured" from "something is configured but empty". That now holds:
one empty + one populated total_count=2 Empty: outage_count=0 outages=[]
N-1: outage_count=2
only empty cases total_count=2
no cases at all total_count=0 results=[]
Dropping the if outages: guard is the whole fix, and outage_count/outages fall out of the existing code rather than needing special-casing. The GetValue comment is accurate and sits directly above the decode, which is where someone extending this would actually be looking.
One side effect, not a blocker
Empty cases now draw from the same max_results budget as populated ones, and can crowd them out:
6 cases (5 empty + 1 populated), max_results=5
total_count: 6 returned_count: 5
names returned: ['Empty0', 'Empty1', 'Empty2', 'Empty3', 'Empty4']
populated case included? NO
The one case that actually has contingencies in it is the one that got truncated away. Before this PR the budget only ever went to populated cases, so this is new.
Discoverable rather than silent — total_count: 6 against returned_count: 5 tells the caller something was cut — and with the default limit of 100 you need more than a hundred fault cases to reach it, so it is a corner. But the truncation now drops the informative rows in preference to the empty ones, which is backwards. A stable sort before the slice fixes it:
fault_cases.sort(key=lambda entry: not entry[1]) # populated firstGetContents ordering is arbitrary anyway, so nothing is lost by imposing one.
Minor
- The docstring does not mention the new behaviour.
"""List available static contingency fault cases and outage events."""is what the MCP client shows a model, and a model deciding whetheroutage_count: 0means "empty" or "failed to read" has only this to go on. One clause — "cases with no outage events are included withoutage_count: 0" — would settle it.PowerFactory/README.md:57is a summary table, so less important. - The validation numbers in the description do not match this repository. A full collection from the repository root is 466 tests (447 passed, 19 skipped here), not "2,198 passed and 25 skipped". A bare
pytestfrom the root does not pick up the in-tree.venv— pytest skips dot-directories — so I could not reproduce that figure and do not know what it covered. Probably a vendored library's own suite got included. Not a problem with the change; flagging it because it is not a signal you can lean on, and the repository's own number is the one CI reports.
Happy to merge. If you would like the one-line ordering change first, say so and I will add it; otherwise it is a fair follow-up for whenever someone next touches the function.
|
Thanks for verifying all three states:
The existing behavior and counts remain intact. I also addressed the noted side effect:
Validation: 32 PowerFactory tests passed, including 3 subtests. |
qian-harvard
left a comment
There was a problem hiding this comment.
Both points are addressed and I can't find anything left. Verified rather than read:
The crowding-out case from my last review is fixed. Same input that previously returned five empty cases and dropped the only populated one:
5 empty + 1 populated, max_results=5
returned: ['N-1', 'Empty0', 'Empty1', 'Empty2', 'Empty3']
populated included? YES
Stability is genuinely preserved, not just claimed:
input : ['E1', 'P1', 'E2', 'P2', 'E3', 'P3']
output: ['P1', 'P2', 'P3', 'E1', 'E2', 'E3']
Original relative order intact inside each group, which is what list.sort being stable buys you.
No regression at the edges. All-empty still reports total_count: 2, none-configured still reports 0, so the distinction the original issue was about still holds. max_results=1 with the empty case first returns ['N-1'] with total_count: 2, returned_count: 1 — truncation still signalled.
The test now guards the sort, which I did not expect and like. Flipping the fake so the empty case comes first makes the pre-existing results[1]["outage_count"] == 0 assertion depend on the ordering working. I confirmed by deleting the sort line: test_contingency_listing_and_bounded_results fails. So the behaviour can't silently regress later, and the new max_results=1 case pins it directly rather than incidentally.
The docstring now reads "List configured fault cases, including cases with no outage events." — which is the string the model actually gets.
Full suite: 447 passed, 19 skipped, unchanged from main. Rebased, green on all four interpreters, diff confined to the two files. Merging. Thanks for the quick turnaround on both rounds.
Summary
Addresses the PowerFactory contingency follow-ups reported in #80.
IntEvtcontingency cases even when they contain noEvtOutageobjects.outage_count: 0andoutages: [].ElmRes.GetValue()returns a(status, value)pair while retaining compatibility with bare scalar values from older PowerFactory versions.Validation
main.PATH.Closes #80