Minimal corrections to Phasor Dynamics models for validation - #538
Minimal corrections to Phasor Dynamics models for validation#538lukelowry wants to merge 10 commits into
Conversation
|
Is this meant to supersede #525 ? |
Yes, I forgot to mention that |
nkoukpaizan
changed the title
nkoukpaizan
left a comment
nkoukpaizan
left a comment
There was a problem hiding this comment.
A few comments. Otherwise looks good!
| RealT ke0 = Ke_; | ||
| if (ke0 == ZERO<RealT>) | ||
| { | ||
| ke0 = (Vrmax_ / 10.0 - static_cast<RealT>(se0)) / static_cast<RealT>(efdp0); | ||
| } |
There was a problem hiding this comment.
I think this still qualifies as changing the input parameter (Ke_), and this is not in the README. Can we avoid it in the PR and successfully validate?
There was a problem hiding this comment.
This is one of the few that I was unsure about in our meeting today. I think that if we instead define a derived parameter as I did in 9082c8a, this will hopefully resolve any confusion. It's not so much changing the parameter as deriving a new parameter. Let me know what you think of my changes
There was a problem hiding this comment.
Understood. In that case, adding this to the Model Derived Parameters section of the documentation should help.
There was a problem hiding this comment.
In #525 parameter Ke_ was overwritten, now the new value is stored in Ke_eff_ and is used instead of Ke_. Unless the new parameter Ke_eff_ has well defined physical meaning and the governing physics is documented in the README file, this change will fall outside the scope of this PR.
abirchfield
left a comment
abirchfield
left a comment
There was a problem hiding this comment.
A good compromise that addresses the issues raised in #525 while allowing us to have excellent validation in the short term. Exciting that we are almost ready to complete the 10k case.
dbbafb2 to
2f1ea87
Compare
|
added specification information and warnings everywhere |
nkoukpaizan
left a comment
nkoukpaizan
left a comment
There was a problem hiding this comment.
I recommend setting Ke_eff_ uniformly in setDerivedParameters and moving the warnings there (instead of in initialize).
6881eff to
c5b70ae
Compare
|
@pelesh @nkoukpaizan Let me know if you think this is ready! Good on my end |
| Log::error() << "Tgov1: initial valve position is outside limits\n"; | ||
| Log::warning() << "Tgov1: initial valve position is outside [Pvmin, Pvmax]. " | ||
| "Check initial dispatch and valve limits\n"; | ||
| return 1; |
There was a problem hiding this comment.
@lukelowry Are you sure about this change? If we are returning 1 and not implementing any correction, this looks like an error to me.
Fine by me... I can always find something to nitpick, but I'll stop there. I'll let @pelesh have the last word. |
4 participants
Description
Validated against PowerWorld. Less than 0.5% relative error on WECC 240 case, ACTIVSg200 case, and ACTIVSg500 case.
Further comments
Other open-source grid modeling frameworks do not even come close to this low a validation error against their validation tools, especially not for cases this large. I don't want perfect to be the enemy of good + we can do cleanup more confidently after merging #461.