-
Notifications
You must be signed in to change notification settings - Fork 85
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
Pid parameters updates #509
Conversation
Something failed when generating plots. See the log at https://github.com/JuliaControl/ControlExamplePlots.jl/runs/847363925?check_suite_focus=true for more details. |
This "reverse-engineering" of the parameters seems like an interesting and quite nice idea. Although I in one way think it is nice that Perhaps it is most convenient (rather than doing this reverse-engineering) if in I think it would be nice if Eventually it would also be nice if one for |
I second this, pretty much 100% of the time I need exactly these two, add a filter and then call ss. Would be nice to have the feature built in for numerical robustness as well as stability in the sense that |
Yeah, I'll update it to this.
Yeah, this was the plan.
Had a go at that, last commit is not complete still but it contains a suggestion on how this could be approached. Not sure how I should introduce it in the form other than standard, so currently I just made a choice which seemed reasonable to me. But opinions are welcome here. The |
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.
Good that there is progress on this!
src/pid_design.jl
Outdated
* `:parallel` - `Kp + Ki/s + Kd*s` | ||
* `:paralleltime` - `Kp + 1/(Ti*s) + Kd*s` | ||
""" | ||
function form2standard(form; params...) |
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.
Shouldn't this function be used in the tf version for pid
, seems like quite duplicated code.
The names for these functions are very non-descriptive. Would be good if they at least contained something with pid
and param
.
Would it be feasible to detect what parameterizaiton the input has based on the fieldnames in the tuples?
Then, one could perhaps get away with only one function pid_params_convert or convert_pid_params?
Are we planning on exporting this function? I would say perhaps not.
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.
Yeah, artifact from the coding process. Wrote pid first, then pidss where I realised I wanted the form2standard. But you are correct, should be used there.
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.
It could be feasible to do that if we made sure all parameter names were unique if they did not correspond to the same thing. Was thinking about it but thought this would not be exported anyway so made this as just some internal helper functions and thought we didn't need to make it more complex than this. They will probably only be used in this file so you have the code and docstring nearby if you every encounter them, but I don't mind a more descriptive name either.
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.
Sure, sounds good.
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.
Even if it is not exported, I think the function name should be better :P
Perhaps also an initial underscore.
_pidṕarams_standard2other
and _pidṕarams_other2standard
would be reasonably descriptive.`
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.
I have made some local updates where it almost is working nicely with detecting parameters, and combining into a single function convert_pid_params(target_form; params...)
. The problem is that I wanted to fix the tests also but got a bit stuck there since pidplots
was not as easy to move over to this format and I could not decide how I wanted to do it, so put it to the side. But can push up what I have and you can have a look. Always good to get some input on how it should be designed.
src/pid_design.jl
Outdated
s = tf("s") | ||
if series | ||
return time ? kp*(one(kp) + one(kp)/(ki*s) + kd*s) : kp*(one(kp) + ki/s + kd*s) | ||
N = get(params, :N, inf) |
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.
Not sure if this is the filtering that we should have (although this is definitely quite common) but the filtering that is used in Advanced PID Control, where different parameterizations are used for PI and PID.
It is definitely quite a mess to handle a lot of different approaches to filtering, but I would say that those two should be the top priority. Perhaps some other ones should be used as well.
How to get this into manageable code seems like a challenge.
There is a problem that different filters are used for PI and PID.
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.
Ahh, okay. Just looked up and took the first I found in the book, but see now that there is an alternative right below. If that form is preferred it seems almost easier to handle since the filter is outside the PI/PID controller. But will have a look at it next time I sit with this.
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.
Is the current version more what you were thinking?
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.
Yes, that looks good, I would perhaps a factor so that the second state corresponds to the filtered control error.
A = [0 1 0; 0 0 1; 0 -2/Tf^2 -2/Tf]
B = [0; 0; 2 / Tf^2]
C = p.Kp * [1/p.Ti 1 p.Td]
D = 0
There is another version that one could consider, but this one looks nice and symmetrical.
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.
Another thing is that one would like a first order filter for pi
controllers. I am not sure how that can be achieved in a good way?
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.
Should it default to a first order filter for PI, or should that be an option? In the Advanced PID I thought they suggested the same filter for both, just different method of setting Tf, but I might have misunderstood.
src/pid_design.jl
Outdated
function convert_pid_params(target; params...) | ||
if haskey(params, :Kc) | ||
Kc = params[:Kc] | ||
τi = get(params, :τi, typeof(Kc)(Inf)) |
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.
This typeof will make it fail when calling with an integer Kc and not supplying taui. Same for Kp/Ti. Not sure if that will be annoying or if there is a nicer way of doing it.
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.
Yes, that would be quite annoying. I would just have wrapped params[:Kc]
in float
, but I am not sure that is the best solution.
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.
You can use typemax
to get the maximum value a type can represent
julia> typemax(Float64)
Inf
julia> typemax(Int)
9223372036854775807
not sure if it makes sense here though, but looking at how the value is used, I think it should be fine. Using float
should probably also be fine since the value will enter a division which will produce floats from ints anyway. Another, perhaps more general solution is to give it the type
Base.promote_op(/, typeof(Kc), typeof(Kc))
julia> Base.promote_op(/, Int, Int)
Float64
which avoids calling float
for types where it does not make sense, e.g., Particles
.
Codecov Report
@@ Coverage Diff @@
## v1 #509 +/- ##
=====================================
Coverage ? 86.86%
=====================================
Files ? 31
Lines ? 3290
Branches ? 0
=====================================
Hits ? 2858
Misses ? 432
Partials ? 0 Continue to review full report at Codecov.
|
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.
Looks good, seems to be on track.
src/pid_design.jl
Outdated
function convert_pid_params(target; params...) | ||
if haskey(params, :Kc) | ||
Kc = params[:Kc] | ||
τi = get(params, :τi, typeof(Kc)(Inf)) |
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.
Yes, that would be quite annoying. I would just have wrapped params[:Kc]
in float
, but I am not sure that is the best solution.
src/pid_design.jl
Outdated
`params` should be supplied with parameter names corresponding to the names used in the above | ||
equations. | ||
""" | ||
function convert_pid_params(target; params...) |
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.
function convert_pid_params(target; params...) | |
function convert_pid_params(target_form; params...) |
consider changing for better clarity
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.
It feels a little bit weird that the parameters are just supplied as kwargs. Perhaps it would be slightly less convenient, but perhaps more natural to provide them as a named tuple in the first argument. Not sure, though.
src/pid_design.jl
Outdated
""" | ||
function stabregionPID(P, ω = _default_freq_vector(P,Val{:bode}()); kd=0, doplot = true) | ||
function stabregionPID(P, ω = _default_freq_vector(P,Val{:bode}()); form=:standard, kd=0) |
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.
Is it necessary to provide `form, can't it be inferred from the provided parameters?
If it is necessary to keep it, I would suggest changing the variable name to pidform
.
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.
We don't supply parameters to this method, we get them back. So it is about what form we want them back in.
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 the other functions yes, but this one seems to produce a plot?
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.
Not sure we are on the same page here.
It produces a plot and returns a tuple of the fig and a vectors of parameters. The form is needed so we know what form the parameters should be returned on. Or am I missing something?
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.
Aha, there are some parameters in the end, I didn't even notice those. Or it is actually a vector of parameters (?), that gives marginally stable closed loop systems.
Although the marginally stabilizing controllers does not seem like too useful of an output, I guess that the selected pidform
should affect the plot, which is currently not the case. I.e., the labels in the plot will be wrong, and the naming of the temporary variables used for fiddling around with the parameters might have misleading names.
kps = map(x->values(x)[1], params)
etc.
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.
It is not indicated what controller parameters that are returned, so if there is a need to return those, that should probably be documented.
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.
Yeah, I honestly didn't really think too much about it since I just wanted to update all the functions to use a similar interface. The plot naming is something that should be done if we allow for a form keyword.
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.
Realised that you send in the D-parameter, so I guess that could be used to figure out the form. If feels like this is getting very messy, not sure I like handling all the forms all the time. Maybe just expose convert_pid_param
instead, and have all our internal functions expect and return values in one of the forms, would be so much easier to handle and probably less confusing for the user to.
src/pid_design.jl
Outdated
end | ||
|
||
|
||
""" | ||
kp,ki,C = loopshapingPI(P,ωp; ϕl,rl, phasemargin, doplot = false) | ||
C, params = loopshapingPI(P, ωp; ϕl, rl, phasemargin, form=:standard, doplot=false) |
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.
C, params = loopshapingPI(P, ωp; ϕl, rl, phasemargin, form=:standard, doplot=false) | |
C, params = loopshapingPI(P, ωp; ϕl, rl, phasemargin, pidform=:standard, doplot=false) |
Alternatively, the user could just convert the output of this function explicitly, not sure if it needs to be lumped together. Also depends on if we are thinking about exporting convert_pid_params
.
src/pid_design.jl
Outdated
kp = [i for i in kps, _ in kis, _ in kds][:] | ||
ki = [j for _ in kps, j in kis, _ in kds][:] | ||
kd = [k for _ in kps, _ in kis, k in kds][:] | ||
kps, kis, kds = kp, ki, kd |
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.
Seems like the Iterators.product
would make this a lot more readable.
For the case below, the vectors could be zipped for consistency. Then it would just be to iterate through the tuples.
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.
Thanks for continuing on this @albheim! I think that things start to become more clear.
Yes, I think we should have some way just supplying three arguments and specifying a form.
kp
, ki
, kd
are extremely evocative of parallel form, so I find the code quite hard to read since they are used so extensively meaning different things. I have a suggestion for how to treat the pid
function in the comments, but for returning parameters couldn't we use named tuples. I don't use them that much, but they seem to be ideal for this? And then also have a method for pid
that accepts named tuples (although it is a bit similar to what we had before).
Another thing that we should think about is how to choose between different types of low-pass filters. One typically don't want to use a second-order filter for PI controllers. This gives even more complications, but at least we should think about how it should be specified.
I'm still not completely sure on how we want the discrete version implemented. Do we implement it so we assume the user has already calculated the desired discrete parameters, or do we calculate some discrete parameters based on continuous ones supplied?
The discrete-time case should be treated separately. I think that just doing c2d
would be okay for now if you are tired of this PR, but I really think that it is something that we want to have done properly, for symbolics and just being sure about what you get.
I just went with ss/tf for now and not arbitrary type, since I felt it was messy enough for now.
Sounds good. What do you mean by arbitrary type? I can only think of zpk
in addition to ss/tf
src/pid_design.jl
Outdated
Calculates and returns a PID controller. | ||
|
||
The `form` can be chosen as one of the following | ||
* `:standard` - `kp*(1 + 1/(ki*s) + kd*s)` |
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.
kp
, ki
, kd
are extremely evocative of parallel form, even if this explanation is good. I think it would be okay if we can't come up with something better, but one suggestions would be along the lines of
kp*(1 + 1/(param_i*s) + param_d*s)
kp + param_i/s + param_d*s
Another option would be p1
, p2
, p3
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.
Changed to param_p, param_i, param_d
now.
src/pid_design.jl
Outdated
""" | ||
pidplots(P, args...; kps=0, kis=0, kds=0, time=false, series=false, ω=0) | ||
function pid_tf(kp, ki, kd; form, Ts, Tf) | ||
kp, ki, kd = convert_pidparams_to_standard(kp, ki, kd, form) |
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.
After you convert to standard form you can write Ti
/Td
, which would avoid a lot of confusion. Typically it is good practice to avoid reusing variable names. It looks a little bit poor with an uppercase letter, but I think it would be alright, otherwise ti
/td
.
I think the input arguments ki
and kd
should be renamed regardless, see above.
I think it would be nice to export the one of pid_tf
/ pid_ss
that is not the default. I have a small preference for that the statespace version should be the default, since I think that we generally encourage working with statespace representaitons, but in this case there is also a good case for the tf
version. But it is not a big deal either way if the other pid_tf
/ pid_ss
is exported. @baggepinnen
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.
I also prefer statespace, but the ss version is only applicable if there is a lowpass filter (which there should always be anyway)
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.
Should there always be a filter as default, even if kd=0? And should this be for tf too?
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.
So all for state space being default? Should I change the keyword to transfer_function=false
then, since it feels better to me at least to trigger a setting by turning it to true. Or maybe just tf=false
to make it nicer to write? Or is it nicer to have a representation=:ss/:tf
keyword?
Currently I export pid_tf
and pid_ss
as well as pid
, not sure if this is needed?
I was thinking this before also, and as you mentioned the PR was looking like that a while ago. But I found it became rather messy and found it clearer to just have the same names of the values in the API at least, internally we can use whatever. I agree that kp, ki and kd are rather connected to the parallel form, but it also felt like the more neutral option to me, it is the constant for p, i and d part.
The filter for now was more to address the issue of the derivative part, and I have not handled it consistently since for ss I skip the filter if kd=0 but not for tf. So this also need to be addressed somehow.
Okay, might go with that then. Otherwise this might never finish. But I agree it would be nice to have something better.
Not clear by me, but @mfalt mentioned at some point that it could be nice to send in some type of |
Maybe it got messy for other reasons, I think this is the right approach, it is just that many variable names should be changed.
|
Both of these are okay with me, not the part I thought got messy really. What I had the most problem with was the use of identifying the form the user wanted by the names of kwargs.
I have no strong opinion here, and in the cases where they are normal arguments I agree. Though there are some places where they are kwargs where it will matter more, and will be breaking, though I don't see that as much of a problem since much of this PR will be breaking anyway. |
This is an automated message.
|
Recently saw It would be nice if they use somewhat similar interface, so I guess that is what should be aimed for. It might be easiest to just restart and skip this PR if anyone wants to have a look, since I doubt I will put much time into it over a reasonable future. |
Something failed when generating plots. See the log at https://github.com/JuliaControl/ControlExamplePlots.jl/actions/runs/2621112747?check_suite_focus=true for more details. |
* Pid parameters updates (#509) * stash * pidparams suggestion * pid creation update (ss) and s2f and f2s * ss default, new filter * remove paralleltime, inf->Inf * update some tests * single conversion method with param detection * update errors * Handle special cases * Update error capitalization * remove gof usage * fix pidplots * Fix pid creation, update stabregionpid * Fix test * Update docs * Fix test pid in tf form * Improve error for pid ss with D-part * Update src/pid_design.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * stash * new interface suggestion * revert some code and make stuff work * Ts=0 to Ts=nothing * change parameter names when standard form * kp/ki/kd -> param_p/param_i/param_d * fix functions missing form * fix tests * fix errors to pass tests * more tests * Ts nothing * make leadlinkcurve recipe and doplot on stabreg * fix some docs Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com> * fixes to pid Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se> Co-authored-by: olof3 <olof3@users.noreply.github.com>
Suggestion of how to handle PID parameters.
The forms @olof3 mentioned in #502 felt familiar to me and they seemed to be somewhat standard, and then extend that with the
paralleltime
version that @baggepinnen was talking about and we get theseKp*(1 + 1/Ti/s + Td*s)
Kc*(1 + 1/τi/s)*(τd*s + 1)
Kp + Ki/s + Kd*s
Kp + 1/Ti/s + Kd*s
To remove the handling of the conversion between different forms it was lifted out of the
placePI
function, and then I also thought maybe we might as well have the user call it separately.Not sure this is desired since I don't use it a lot myself. Is it preferred to have that both params and controller are always returned or should controller be returned from design and then you can extract parameters in desired form after?
Either way, this is not completed. It is just implemented for one method and not integrated with the rest since I didn't know how we wanted it.