IEEEST Stabilizer Improvements/Corrections - #460
Conversation
|
@pelesh this fix is also ready to be merged I forgot to make a PR for this one. |
509506c to
e2c07d8
Compare
pelesh
left a comment
There was a problem hiding this comment.
The logic governing changing differential into algebraic equations and vice versa does not seem to be sustainable. This requires more offline discussion.
e2c07d8 to
bab57ad
Compare
8bbbad6 to
090289b
Compare
|
I have rebased and adjusted the formulation to align with #463 . |
755ee20 to
282fc1d
Compare
nkoukpaizan
left a comment
There was a problem hiding this comment.
I have a few comments here. Perhaps worth discussing at our next meeting.
| using Variable = typename ModelDataT::MonitorableVariables; | ||
| monitor_->set(Variable::vss, [this] | ||
| { return y_[11]; }); | ||
| auto index = [](IeeestInternalVariables variable) |
There was a problem hiding this comment.
I've seen this pattern a few times. Consider defining once if it is really needed. I would argue it's less readable.
282fc1d to
579b3c1
Compare
|
Testing Ground for template parameters |
579b3c1 to
9fb15fa
Compare
|
general implementation of the template parameter done |
7336d1f to
1104b91
Compare
d0dc95a to
d2be200
Compare
|
@nkoukpaizan what are your thoughts on my template implementation? |
The model implementation is cleaner with the |
|
@nkoukpaizan we could possibly require the order to be passed as a parameter of IEEEST model, would that resolve your concern? "Order" is not usually directly exposed as a model parameter and so would require some knowledge of how IEEEST works to pass valid parameters. Can you give an example where the runtime comparison is bad/fails? I might be able to resolve both our concerns if I understand better. |
Yes, exposing the order as a parameter to the model is what I had in mind. Understood that that's not the domain practice. That's why as was asking whether this is possible as a question. Floating point values are stored with limited precision. We'd have to guarantee that the value returned by the parser is bit-for-bit equal to zero. That may be the case for nlohmann-json, but considering #227 and the possibility of using a different parser, that's not guaranteed. |
|
@nkoukpaizan i see the issue now thank you, that was very clarifying. After reflecting on my past experience and discussing with others at TAMU, I think deriving the order from the parameters alone is not intuitive or safe, either. As discussed, we should add an integer parameter to the IEEEST model, "order" and use that for the StabilizerFactory (rename to IeeestFactory). Then, our parameter validation can gate for zero division relevant to each static order (e.g. I will make this change then we can reasess. |
d2be200 to
47a31f1
Compare
735d21d to
5a00d48
Compare
|
Since IEEEST is already not in Hessenberg form, I am temporarily closing this in favor of #520, which is a significantly cleaner implicit representation of the model. The 'minimum time constant' approach is unfortunately making validation quite difficult. Tracked in #463 I am justifying this decision because IEEEST can simply be removed from a case if it becomes a blocker for Hessenberg, but right now validation is more urgent. We can reopen after v0.2 is released |
|
Rebasing ASAP |
|
I suggest to proceed like this:
I would avoid templates for now. I don't think we should use hacks as in #520. |
5a00d48 to
df117f0
Compare
|
@pelesh please see most recent version of this branch, where I use a switch. How does this look? |
|
Confirmed working in validation of three cases against PowerWorld. See #461 |
e9ebe94 to
d81ab35
Compare
pelesh
left a comment
There was a problem hiding this comment.
Looks much better!!
A few things to fix quickly before more thorough review.
nkoukpaizan
left a comment
There was a problem hiding this comment.
A few comments on the current state of the PR.
I'll defer to @pelesh if the switch really is what we want to go with.
| switch (order_) | ||
| { | ||
| case 0: |
There was a problem hiding this comment.
I'm not sure having the switch inside the residual evaluation is better than other alternatives we've explored. I suspect that is the motivation for the new -mllvm -simplifycfg-sink-common=false flag.
There was a problem hiding this comment.
Yes I had to add that flag for Enzyme to construct correctly. I will wait for @pelesh to chime in
There was a problem hiding this comment.
No, this is not going to work as implemented (I rushed to celebration too early). You need a separate residual for each order and separate Jacobian for each residual. Jacobian derivatives are evaluated at compile time, so they have to have residual equations available at compile time.
The switch statement can be used to select residual function of the right order and its matching Jacobian but they need to be implemented separately. The rest of model infrastructure could be reused, I think.
|
@nkoukpaizan thank you for the timely review. pending @pelesh and @abirchfield now |
292c07c to
d993220
Compare
pelesh
left a comment
There was a problem hiding this comment.
The current implementation does not build on my machine. Enzyme seems to be trying to get derivatives for all runtime combinations and eventually runs out of memory.
The issue we are dealing here is not the modeling of a device but rather trying to fit five different mathematical model in one component. The documentation in README is misleading in that regard.
I suggest implement stabilizer for only one order as a temporary solution.
d993220 to
aa89aaa
Compare
Description
Fixes and cleans up the
IEEESTstabilizer implementation and documentation.This updates the older initial implementation style so that reduced-order notch filters and zero denominator time constants are handled consistently
Proposed changes
IEEESTfrom the attached input signal instead of forcing all states to zero.Checklist
-Wall -Wpedantic -Wconversion -Wextra.Changelog changes N/A
Further comments
This is cleanup/correction work for the existing
IEEESTmodel, not a new model