-
Notifications
You must be signed in to change notification settings - Fork 7
Modify Smooth Approximation of Limits #313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@lukelowry Thanks for this. I likely didn't make myself clear in the meeting last week. The challenging part for Enzyme is not the sigmoid, but the saturation: the following branching in the exciter model.
|
Ah, I see now. Fixed in the recent commit. Please advise if this is sufficient for Enzyme. |
|
Looks like the new sigmoid is more challenging to differentiate (sharper approximation?) than the previous one. One of the derivatives is The previous result was: |
Just take a look at parameters -- the code is using |
|
@lukelowry, what is the lowest value of |
|
@pelesh @nkoukpaizan Hard to say what the threshold is, but since the limits are on the order of We can change it back to the absolute value form, if needed. |
I think exponential form is better, one just need to be mindful of the exponential growth rate. |
|
From the derivative's perspective, the function is well-behaved up to |
|
I have confirmed that removing the branching gets me past the Enzyme build error I was seeing. I'm seeing another error that is unrelated to the model implementation, and I can work around that. For this PR, I would just suggest setting a lower |
3ab969c to
81b49f2
Compare
nkoukpaizan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I made the latest changes, it would be good to have another pair of eyes (@pelesh @abirchfield ) on this before merging.
pelesh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason exciter tests fail for me:
The following tests FAILED:
25 - GenrouTest1_Ieeet1_Json (Subprocess aborted)
26 - GenrouTest1_Ieeet1_Json_no_arg (Subprocess aborted)
Errors while running CTest
Output from these tests are in: /Users/55y/src/gridkit/build-gcc/Testing/Temporary/LastTest.log
|
I am not sure how this passed CI pipeline: |
|
@PhilipFackler, I believe the check here may be overzealous:
If I comment out the exception, everything works correctly. I don't think that at this point we can check if a governor is connected to the generator because governor may have not been allocated/connected at this point. Also, we may need to discriminate here between sensors and actuators. CC @nkoukpaizan |
Description
Change the smooth limit approximation used in the IEEET1 Exciter and TGOV1 Governor so that it is twice differentiable. Additionally, the exciter saturation equation now has a smooth approximation. This is for Enzyme compatibility.
Proposed changes
The logistic function is now used instead of the approximation of the logistic function
Checklist
-Wall -Wpedantic -Wconversion -Wextra.Further Comments
Needed for Enzyme to work on larger cases