JuliaControl / ControlSystems.jl

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

truncate T in baltrunc #864

Closed baggepinnen closed 1 year ago

JuliaControlBot commented 1 year ago
This is an automated message. Plots were compared to references. 11/11 images have changed, see differences below. After pulling this PR, please update the reference images by creating a PR to ControlExamplePlots.jl here. Difference Reference Image New Image
:x: 0.047 Reference New
:heavy_check_mark: 0.011 Reference New
:heavy_check_mark: 0.001 Reference New
:heavy_check_mark: 0.001 Reference New
:heavy_check_mark: 0.007 Reference New
:heavy_check_mark: 0.003 Reference New
:heavy_check_mark: 0.0 Reference New
:warning: 0.028 Reference New
:x: 0.06 Reference New
:heavy_check_mark: 0.0 Reference New
:warning: 0.017 Reference New
codecov[bot] commented 1 year ago

Codecov Report

Merging #864 (3192df3) into master (fd2e732) will not change coverage. Report is 2 commits behind head on master. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #864   +/-   ##
=======================================
  Coverage   92.72%   92.72%           
=======================================
  Files           5        5           
  Lines         330      330           
=======================================
  Hits          306      306           
  Misses         24       24           
hexaeder commented 11 months ago

This should have been released as part of ControlSystemsBase 2.0 (and ControlSystems 2.0 for that matter) since it is a breaking change... I was actually using the full balancing transformation T for further analysis.

Just a minor gripe though, thanks for building all the ControlSystems infrastructure in the first place :)

baggepinnen commented 11 months ago

Oops, sorry about that! According to the (old) docstring, the documented properties of T were

T is the similarity transform between the old state x and the new state z such that Tz = x.

and with that definition, the old behavior was incorrect, since z was the reduced-order state, so this was intended as a bug fix. The docstring wasn't actually completely accurate either, since the transformation was in the direction z = Tx, so the PR fixed that as well.

If you are after the balancing transform, you have the function balreal, but calling both balreal and baltrunc incurs double solutions of the Lyapunov equations which can be expensive for large systems, if it becomes a problem, we can perhaps add an option to baltrunc to keep the full transformation matrix?

hexaeder commented 11 months ago

At least it was a similarity transform before! Jokes aside, I think it makes sense to return the reduced T and this behavior is more aligned with the docstring. I was just confused for a moment. In my case, I can just do the balancing twice, the system is small anyways, so there is no problem and my code is already updated :)