JuliaControl / ControlSystems.jl

A Control Systems Toolbox for Julia
https://juliacontrol.github.io/ControlSystems.jl/stable/
Other
510 stars 85 forks source link

Convert pid parameters to :parallel #900

Closed mzaffalon closed 10 months ago

mzaffalon commented 11 months ago

Using standard fails when KP=0.

mzaffalon commented 11 months ago

Addresses #897.

codecov[bot] commented 11 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mzaffalon commented 11 months ago

Two questions:

  1. how can I increase code coverage?
  2. what is the purpose of @. in, say, https://github.com/JuliaControl/ControlSystems.jl/blob/master/lib/ControlSystemsBase/src/pid_design.jl#L559
baggepinnen commented 10 months ago
  1. By adding tests that causes the added lines of code to be executed.
  2. image
mzaffalon commented 10 months ago
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 commented 10 months ago

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 commented 10 months ago

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

baggepinnen commented 10 months ago

Very nice, thanks for your hard work :)

mzaffalon commented 10 months ago

My pleasure and thank you for your patience.