Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Contingency parsing can still select the wrong model because BusTrip and BusFault remain ambiguous.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds contingency validation and logging to prevent conflicting contingency parsing.
Changes:
- Rejects unknown contingency fields.
- Explicitly parses machine-trip contingencies.
- Logs parsed contingency types.
File summaries
| File | Summary |
|---|---|
pypsse/simulator.py |
Logs parsed contingency types. |
pypsse/models.py |
Critical (3 votes): BusTrip payloads remain ambiguous with BusFault; explicit discrimination is required. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| class BusFault(BaseModel): | ||
| "Bus fault model defination" | ||
| model_config = ConfigDict(extra="forbid") |
| model_config = ConfigDict(extra="forbid") | ||
|
|
||
| time: float = 0.2 | ||
| bus_id: int = 38205 |
There was a problem hiding this comment.
all other trips are just a list of ints, why is this one a specific int?
There was a problem hiding this comment.
I could see upcoming use cases where durations were needed for all faults/trips when modeling restoration. Is there a different reason you added the ConfigDict(extra="forbid")?
I could also see this messing with other branches if others have built upon these classes.
Isn't it on the user to prevent conflicting contingencies in their input file? Let me know if I'm missing something, but I'm not sure this change belongs in main.
Adds a field validator to prevent the scenario where passing one parameter as part of one contingency triggers anther unexpectedly. Only use a contingency if all required parameters are present.