Repository navigation
Complete PowerFactory contingency recording and screening - #95
aswinkrishnapoyil wants to merge 1 commit into
Conversation
qian-harvard
left a comment
There was a problem hiding this comment.
Thanks. The remove tool and the screening configuration fill real gaps, and treating the IntMon selections as the source of truth to get past stale ElmRes columns makes sense.
I reviewed this against the fakes, since I don't have PowerFactory here. The inline questions about PowerFactory behaviour need your live setup.
Before merging, please address:
- A failed removal is reported as
already_absent(remove_contingency_result_variables). - One unreadable
IntMonfails the whole add call, and remove reports it once per object. - Whether class-level
IntMonselections can exist. If they can, both tools miss them.
The rest is smaller: the list[Any] schema change, missing attributes reported as False, test coverage of the IntMon path, duplication between add and remove, and the configure tool's description.
| "variable": variable, | ||
| "code": code, | ||
| }) | ||
| (removed if removed_here else already_absent).append(variable) |
There was a problem hiding this comment.
A failed removal ends up in already_absent. If RemoveVar raises, or returns something other than 0/None/1, for every matching monitor, removed_here stays False and the variable is filed as already absent.
With one monitor whose RemoveVar raises, the result is already_absent_variables: 1 and the message "Some result variables could not be removed; all requested variables were already absent; no change", while the variable is still recorded.
Suggest a third per-variable outcome (e.g. failed). Count a variable as absent only when every matching monitor returned 1, or no monitor matched.
| recorded = {} | ||
| for monitor in monitors: | ||
| monitored_object = monitor.GetAttribute("obj_id") | ||
| if monitored_object is None: |
There was a problem hiding this comment.
Question, since I can't check this without PowerFactory: can an IntMon in the result file select variables for a whole class (empty obj_id, class name set)?
If it can, both tools skip it here. Add would report a class-recorded m:u as added and ask for a rerun. Remove would report it as already_absent while it is still recorded. The FindColumn check this replaces would have seen those columns. If class-level selections are possible, they should count as recording the variable for every object of that class, or at least be reported.
|
|
||
| recorded = {} | ||
| for monitor in monitors: | ||
| monitored_object = monitor.GetAttribute("obj_id") |
There was a problem hiding this comment.
An exception from any one monitor (GetAttribute("obj_id"), NVars, GetVar, GetFullName) escapes this helper. It fails the whole add call as "PowerFactory read failed" before anything is configured, which undoes the per-object error scoping from #89.
In remove_contingency_result_variables, the same broken monitor is reported once per queried object instead: 3 identical errors for a 3-bus query, and up to 1000 with max_objects=1000. Suggest catching per monitor here, collecting those errors once, and having both tools report them the same way.
| def add_contingency_result_variables( | ||
| object_query: str, | ||
| variables: list[str], | ||
| variables: list[Any], |
There was a problem hiding this comment.
The PR description says list[Any] keeps numbers from being coerced to strings, but pydantic v2 doesn't do that coercion. With mcp 2.3.0 and list[str], ["m:u", 7, ""] is already rejected with variables.1 Input should be a valid string.
What list[Any] does change is the tool schema: the items go from {"type": "string"} to {}, so models lose the type hint. I'd keep list[str] here and in remove_contingency_result_variables. _contingency_variable_names is still worth keeping for blank and whitespace-only entries, which pydantic accepts.
| "screening_method": {0: "dc", 1: "ac_linearised"}.get( | ||
| method, "unknown" if method is None else f"unknown:{method}" | ||
| ), | ||
| "simple_loading_criterion": bool(_attribute(command, "scrCritSimple", 0)), |
There was a problem hiding this comment.
On a PowerFactory build without these attributes, _attribute returns the default 0 and bool() turns it into False. So get_contingency_configuration reports the four criteria as disabled when they don't exist, while the thresholds correctly come back None.
Suggest returning None for missing booleans too, e.g. a small helper that returns None when the attribute is absent and bool(value) otherwise.
|
|
||
|
|
||
| @mcp.tool() | ||
| def remove_contingency_result_variables( |
There was a problem hiding this comment.
Most of this function's first ~50 lines duplicate add_contingency_result_variables: the method, variables and max_objects checks, then study case, ComSimoutage, result file and object query. It also re-implements the monitor-to-object matching instead of reusing an index like _contingency_recorded_variables.
The two already disagree. Add silently skips an object whose name can't be read, while remove reports it, and add has a fallback that remove doesn't. A shared helper for the preamble and one that maps full name to monitors would keep them aligned.
| continue | ||
|
|
||
| matching_monitors = [] | ||
| for monitor in monitors: |
There was a problem hiding this comment.
This calls GetAttribute("obj_id") and GetFullName() for every monitor, once per object, so it makes O(objects × monitors) PowerFactory API calls on the single PF thread. With 1000 objects and ~1000 selections, that's about 2 million calls, and other tools wait behind them.
Building the full-name-to-monitors map once before the object loop, as add does, makes it linear.
| ignore_base_case_overloads: bool | None = None, | ||
| screen_only_recorded_elements: bool | None = None, | ||
| ) -> str: | ||
| """Persist selected ComSimoutage screening settings without executing it.""" |
There was a problem hiding this comment.
This docstring is the tool description the model sees. Since this tool changes persistent study-case state, it needs more than one line. Please document each argument:
- what
dcvsac_linearisedscreening means - that the thresholds are percentages
- that
None(or""for the method) leaves a setting unchanged - that the change persists in the study case
The neighbouring contingency tools are a good model.
| for monitor in monitors: | ||
| try: | ||
| monitored_object = monitor.GetAttribute("obj_id") | ||
| if monitored_object is obj or ( |
There was a problem hiding this comment.
Minor: unless the Python API reuses wrapper objects, monitored_object is obj only matches in the test fakes, and in PowerFactory the full-name comparison always decides. Comparing by full name alone, with a test that uses distinct-but-equal objects, would exercise the real path.
| "query": query, | ||
| "variables": variable_names, | ||
| "total_objects": len(objects), | ||
| "configured_objects": len(configured), |
There was a problem hiding this comment.
configured gets every queried object, including ones with no matching IntMon, so configured_objects is the number of objects examined. A query over 50 buses where 2 have selections reports 50, with 48 counted as already_absent.
In add, the same field counts only objects that were touched. Suggest counting only objects with a matching selection here, or reporting objects_without_selection separately.
Context
This PR follows up on #89 and addresses the two optional items identified during its review:
add_contingency_result_variableshad no inverse operation.It also adds persistent configuration of PowerFactory contingency-screening criteria.
Changes
Complete the result-recording lifecycle
remove_contingency_result_variables.IntMonselections.ElmRescolumn inspection as a compatibility fallback.Handle stale
ElmRescolumnsLive PowerFactory testing exposed an important edge case:
IntMonrecording selection.ElmRescolumn remains visible until the contingency analysis is rerun.The add tool now treats the
IntMonselections as the source of truth. This allows a removed variable to be added back immediately without running a calculation first.Configure contingency screening
Add
configure_contingency_screening, which can persist selectedComSimoutagescreening settings without executing the analysis:The tool:
get_contingency_configurationnow also returns these screening settings.Documentation
Updated:
PowerFactory/README.mdPowerFactory/functions_overview.txtAutomated validation
PowerFactory suite