Skip to content

Expose empty PowerFactory contingency cases - #83

Merged
qian-harvard merged 2 commits into
Power-Agent:mainfrom
aswinkrishnapoyil:fix/powerfactory-contingency-followups
Sep 21, 2026
Merged

qian-harvard merged 2 commits into
Power-Agent:mainfrom
aswinkrishnapoyil:fix/powerfactory-contingency-followups

Conversation

@aswinkrishnapoyil

@aswinkrishnapoyil aswinkrishnapoyil commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Addresses the PowerFactory contingency follow-ups reported in #80.

  • Include configured IntEvt contingency cases even when they contain no EvtOutage objects.
  • Represent empty cases with outage_count: 0 and outages: [].
  • Document that ElmRes.GetValue() returns a (status, value) pair while retaining compatibility with bare scalar values from older PowerFactory versions.
  • Add regression coverage for contingency cases without outage events.

Validation

  • PowerFactory focused suite: 32 passed, including 3 subtests.
  • Repository CI set: 447 passed and 19 skipped, unchanged from main.
  • The two Git Bash-dependent GenX tests passed separately after adding Git Bash to PATH.

Closes #80

@qian-harvard qian-harvard left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 first

GetContents 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 whether outage_count: 0 means "empty" or "failed to read" has only this to go on. One clause — "cases with no outage events are included with outage_count: 0" — would settle it. PowerFactory/README.md:57 is 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 pytest from 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.

@aswinkrishnapoyil

aswinkrishnapoyil commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for verifying all three states:

  • A mixture of empty and populated cases.
  • Only empty configured cases.
  • No configured cases.

The existing behavior and counts remain intact.

I also addressed the noted side effect:

  • Added stable populated-first ordering before applying max_results, preventing empty cases from crowding populated cases out of a truncated response.
  • Preserved the original relative order within the populated and empty groups.
  • Updated the tool docstring to explicitly state that configured cases with no outage events are returned.
  • Added regression coverage where an empty case appears first and max_results=1; the populated case is still returned.

Validation: 32 PowerFactory tests passed, including 3 subtests.

@qian-harvard qian-harvard left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@qian-harvard
qian-harvard merged commit ca420c3 into Power-Agent:main Sep 21, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PowerFactory contingency tools: empty fault cases are invisible, GetValue heuristic undocumented

2 participants