Convert pid parameters to :parallel #900

mzaffalon · 2023-11-11T22:56:40Z

Using standard fails when KP=0.

Using standard fails when KP=0

mzaffalon · 2023-11-11T22:57:00Z

Addresses #897.

codecov · 2023-11-11T23:13:04Z

Codecov Report

Attention: 57 lines in your changes are missing coverage. Please review.

Comparison is base (6e6a0d1) 34.06% compared to head (0ac6a75) 34.03%.
Report is 3 commits behind head on master.

Files	Patch %	Lines
lib/ControlSystemsBase/src/pid_design.jl	0.00%	57 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

mzaffalon · 2023-11-12T08:53:00Z

Two questions:

how can I increase code coverage?
what is the purpose of @. in, say, https://github.com/JuliaControl/ControlSystems.jl/blob/master/lib/ControlSystemsBase/src/pid_design.jl#L559

baggepinnen · 2023-11-13T05:47:52Z

By adding tests that causes the added lines of code to be executed.

baggepinnen · 2023-11-13T05:51:12Z

lib/ControlSystemsBase/src/pid_design.jl

@@ -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)


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.

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.

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 public keyword) 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

mzaffalon · 2023-11-13T08:52:47Z

2. ![image](https://user-images.githubusercontent.com/3797491/282374098-731b5403-32d5-4968-8edb-998e8ec99470.png)

Thank you. So it has no use on this specific line?

baggepinnen · 2023-11-13T09:07:07Z

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

mzaffalon · 2023-11-13T09:12:37Z

Thank you. I hope I didn't forget any.

baggepinnen · 2023-11-13T09:16:33Z

lib/ControlSystemsBase/src/pid_design.jl

-            (Ti - sqrt(Ti * (Ti - 4 * Td))) / 2,
-            (Ti + sqrt(Ti * (Ti - 4 * Td))) / 2,
-        )
+        Δ = Ti * (Ti - 4 * Td)


These lines are broken for parameter arrays

lib/ControlSystemsBase/src/pid_design.jl

baggepinnen · 2023-11-13T09:18:28Z

lib/ControlSystemsBase/src/pid_design.jl

+"""
+function convert_pidparams_from_parallel(Kp, Ki, Kd, to::Symbol)
+    if to === :parallel
+        return @. (Kp, Ki, Kd)


Suggested change

return @. (Kp, Ki, Kd)

return Kp, Ki, Kd

baggepinnen · 2023-11-13T09:19:39Z

lib/ControlSystemsBase/src/pid_design.jl

+        Δ < 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))


The @. here is not enough, the lines above, like

Δ = Kp^2-4Ki*Kd Δ < 0 && ...

would fail for arrays

Would you rather have Julia throw a DomainError here? In general to me it makes more sense to convert single pids at the time.

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?

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 <baggepinnen@gmail.com>

lib/ControlSystemsBase/test/test_pid_design.jl

Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com>

baggepinnen · 2023-11-17T09:13:01Z

Very nice, thanks for your hard work :)

mzaffalon · 2023-11-17T14:28:01Z

My pleasure and thank you for your patience.

Convert pid parameters to :parallel

a3b77eb

Using standard fails when KP=0

Throw DomainError when conversion to other form is not possible

27a0c18

baggepinnen reviewed Nov 13, 2023

View reviewed changes

Reintroduce convert_pidparams_from_standard

a43c256

Expand tests

Re-add broadcasting

692b5f9

baggepinnen reviewed Nov 13, 2023

View reviewed changes

mzaffalon and others added 3 commits November 13, 2023 10:28

Update lib/ControlSystemsBase/src/pid_design.jl

c48eda3

Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com>

Remove broadcasting on convert_pidparams* functions

9908eaf

merge

5392ea1

baggepinnen reviewed Nov 17, 2023

View reviewed changes

lib/ControlSystemsBase/test/test_pid_design.jl Show resolved Hide resolved

Update lib/ControlSystemsBase/test/test_pid_design.jl

0ac6a75

Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com>

baggepinnen merged commit 561743c into JuliaControl:master Nov 17, 2023
3 of 5 checks passed

mzaffalon deleted the zaf/pid_parallel_form branch November 17, 2023 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert pid parameters to :parallel #900

Convert pid parameters to :parallel #900

mzaffalon commented Nov 11, 2023

mzaffalon commented Nov 11, 2023

codecov bot commented Nov 11, 2023 •

edited

Loading

mzaffalon commented Nov 12, 2023

baggepinnen commented Nov 13, 2023

baggepinnen Nov 13, 2023

albheim Nov 13, 2023

baggepinnen Nov 13, 2023

mzaffalon commented Nov 13, 2023 •

edited

Loading

baggepinnen commented Nov 13, 2023 •

edited

Loading

mzaffalon commented Nov 13, 2023

baggepinnen Nov 13, 2023

baggepinnen Nov 13, 2023

baggepinnen Nov 13, 2023

mzaffalon Nov 13, 2023

baggepinnen Nov 13, 2023

baggepinnen Nov 13, 2023 •

edited

Loading

baggepinnen commented Nov 17, 2023

mzaffalon commented Nov 17, 2023

Convert pid parameters to :parallel #900

Convert pid parameters to :parallel #900

Conversation

mzaffalon commented Nov 11, 2023

mzaffalon commented Nov 11, 2023

codecov bot commented Nov 11, 2023 • edited Loading

Codecov Report

mzaffalon commented Nov 12, 2023

baggepinnen commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzaffalon commented Nov 13, 2023 • edited Loading

baggepinnen commented Nov 13, 2023 • edited Loading

mzaffalon commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baggepinnen Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

baggepinnen commented Nov 17, 2023

mzaffalon commented Nov 17, 2023

codecov bot commented Nov 11, 2023 •

edited

Loading

mzaffalon commented Nov 13, 2023 •

edited

Loading

baggepinnen commented Nov 13, 2023 •

edited

Loading

baggepinnen Nov 13, 2023 •

edited

Loading