Convert pid parameters to :parallel #900
Conversation
Using standard fails when KP=0
|
Addresses #897. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #900 +/- ##
==========================================
- Coverage 34.06% 34.03% -0.03%
==========================================
Files 42 42
Lines 4624 4642 +18
==========================================
+ Hits 1575 1580 +5
- Misses 3049 3062 +13 ☔ View full report in Codecov by Sentry. |
|
Two questions:
|
|
| @@ -511,26 +509,26 @@ function loopshapingPID(P0, ω; Mt = 1.3, ϕt=75, form::Symbol = :standard, dopl | |||
| end | |||
|
|
|||
| """ | |||
| Kp, Ti, Td = convert_pidparams_to_standard(param_p, param_i, param_d, form) | |||
| Kp, Ti, Td = convert_pidparams_to_parallel(param_p, param_i, param_d, form) | |||
There was a problem hiding this comment.
It would be better to keep the old function as it was and just add the new function. We don't want to unnecessarily break any downstream package that might have use this function.
There was a problem hiding this comment.
On the phone so haven't checked, but iirc we neither have this in online docs or export it, so I probably wouldn't count it as api.
I think the thing that is exported is the convert_pidparams_from_to, though I could be wrong.
But it is also easy to leave it as an extra method if you feel more comfortable with that.
There was a problem hiding this comment.
I'd like to keep it for a few reasons
- I make use of it in some downstream packages
- It would be nice to keep the old version around for comparison for a while in case we encounter any strange results with the new method
- Julia currently (as of v1.9, but soon improved by the new
publickeyword) lacks a good way to indicate what is and isn't public facing API, and whether or not something is included in the docs is a policy that is awkward to work with from a user perspective. I thus wouldn't be surprised if other people have found this function and made use of it. It does have a nice docstring after all, and we haven't explicitly documented any policy for what is and isn't public API in ControlSystems.
Expand tests
Thank you. So it has no use on this specific line? |
|
It did have a use, it supported parameter arrays as inputs julia> ControlSystemsBase.convert_pidparams_from_standard(rand(3), rand(3), rand(3), :parallel)
([0.17773398598165757, 0.32161078829707446, 0.722649466067516], [0.2515777728255106, 0.36780970052897355, 0.7964910544026514], [0.09807961789854291, 0.2953669775189507, 0.042697386412817936])with the refactoring currently applied in this PR, that functionality was broken. One could argue whether that functionality was needed or not, but since it was there we shouldn't break it |
|
Thank you. I hope I didn't forget any. |
| (Ti - sqrt(Ti * (Ti - 4 * Td))) / 2, | ||
| (Ti + sqrt(Ti * (Ti - 4 * Td))) / 2, | ||
| ) | ||
| Δ = Ti * (Ti - 4 * Td) |
There was a problem hiding this comment.
These lines are broken for parameter arrays
| """ | ||
| function convert_pidparams_from_parallel(Kp, Ki, Kd, to::Symbol) | ||
| if to === :parallel | ||
| return @. (Kp, Ki, Kd) |
There was a problem hiding this comment.
| return @. (Kp, Ki, Kd) | |
| return Kp, Ki, Kd |
| Δ < 0 && | ||
| throw(DomainError("The condition Kp^2 ≥ 4Ki*Kd is not satisfied: the PID parameters cannot be converted to series form")) | ||
| sqrtΔ = sqrt(Δ) | ||
| return @. ((Kp - sqrtΔ)/2, (Kp - sqrtΔ)/(2Ki), (Kp + sqrtΔ)/(2Ki)) |
There was a problem hiding this comment.
The @. here is not enough, the lines above, like
Δ = Kp^2-4Ki*Kd
Δ < 0 && ...would fail for arrays
There was a problem hiding this comment.
Would you rather have Julia throw a DomainError here? In general to me it makes more sense to convert single pids at the time.
There was a problem hiding this comment.
You could check
any(<(0), Δ)
In general to me it makes more sense to convert single pids at the time.
I agree, that would probably be a nicer implementation. I performed some tests locally, and it does not seem like most PID-related functions actually handle array parameters, and I cannot remember why it was supported in the first place. Maybe it's safe to assume that this functionality isn't used anywhere and not bother supporting it?
There was a problem hiding this comment.
Ah, I see now that stabregionPID does pass in array parameters, so if we drop support for array parameters this function would need an update
Co-authored-by: Fredrik Bagge Carlson <[email protected]>
Co-authored-by: Fredrik Bagge Carlson <[email protected]>
|
Very nice, thanks for your hard work :) |
|
My pleasure and thank you for your patience. |
Using standard fails when KP=0.