Return a result structure from lsim by baggepinnen · Pull Request #529 · JuliaControl/ControlSystems.jl

baggepinnen · 2021-05-29T08:43:30Z

This is by design a non-breaking change to the lsim interface since the structure allows both getindex and destructuring to behave just like if lsim returned a tuple of arrays like before. Indeed, the tests for lsim were not touched in this commit.

This commit also adds a plot recipe to the result structure. All plot recipes for lsimplot, stepplot, impulseplot have been replaced by the new recipe. This is a breaking change since the names of the previous plots no longer exist. A slight change is that the plots for a step response no longer show the text "step response", but I think that's an acceptable change, the user can supply any title they prefer themselves.

Any form of interpolation etc. can be added later if we find that useful, but I think this is enough for an initial PR. The structure contains all the outputs of lsim together with the system that was used to produce the results.

This is by design a non-breaking change to the lsim interface since the structure allows both getindex and destructuring to behave just like if lsim returned a tuple of arrays like before. Indeed, the tests for lsim were not touched in this commit. This commit also adds a plot recipe to the result structure. All plot recipes for lsimplot, stepplot, impulseplot have been replaced by the new recipe. This is a breaking change since the names of the previous plots no longer exist. A slight change is that the plots for a step response no longer show the text "step response", but I think that's an acceptable change, the user can supply any title they prefer themselves.

JuliaControlBot · 2021-05-29T08:51:51Z

Something failed when generating plots. See the log at https://github.com/JuliaControl/ControlExamplePlots.jl/runs/887864520?check_suite_focus=true for more details.

JuliaControlBot · 2021-05-29T08:54:07Z

Something failed when generating plots. See the log at https://github.com/JuliaControl/ControlExamplePlots.jl/runs/887869037?check_suite_focus=true for more details.

codecov · 2021-05-29T08:57:32Z

Codecov Report

Merging #529 (5c00751) into dev (f644db6) will increase coverage by 0.01%.
The diff coverage is 94.73%.

❗ Current head 5c00751 differs from pull request most recent head 0088980. Consider uploading reports for the commit 0088980 to get more accurate results

@@            Coverage Diff             @@
##              dev     #529      +/-   ##
==========================================
+ Coverage   85.08%   85.10%   +0.01%     
==========================================
  Files          31       32       +1     
  Lines        3192     3188       -4     
==========================================
- Hits         2716     2713       -3     
+ Misses        476      475       -1

Impacted Files	Coverage Δ
src/ControlSystems.jl	`100.00% <ø> (ø)`
src/types/result_types.jl	`89.47% <89.47%> (ø)`
src/plotting.jl	`76.41% <95.00%> (-0.65%)`	⬇️
src/simplification.jl	`100.00% <100.00%> (ø)`
src/timeresp.jl	`91.79% <100.00%> (+0.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0c960a...0088980. Read the comment docs.

olof3

Nice, seems like the right way to go.

A slight change is that the plots for a step response no longer show the text "step response", but I think that's an acceptable change

Seems acceptable. One could possibly pass it along in the struct, it is somewhat nice to see at least. Not sure what happens if multiple response types are used in the same plot.

together with the system that was used to produce the results.

Are you thinking about using it for interpolation, otherwise it seems more clear to only keep the information that is used, i.e. continuous/discrete (?).

I guess it would be nice to be able to use the plot recipe with data that was user generated, e.g., by some method outside of ControlSystems, i.e., not having to specify a system, but that can of course be achieved regardless.

In general, even without a system type, I guess there could be quite a bit of compilation of the recipe since there might be quite a few different types of SimResult that are generated. Not sure how much of a problem this might be?

Any form of interpolation etc. can be added later if we find that useful,

Yes, I definitely think we should wait until there is a very clear need for it.

olof3 · 2021-05-29T09:09:24Z

src/plotting.jl

+@recipe function simresultplot(r::SimResult; plotu=false)
+    ny, nu = r.ny, r.nu
+    t = r.t
+    multiseries = size(r.y, 3) # step and impulse produce multiple results


Would be helpful with a more descriptive name, nseries, nresponses

olof3 · 2021-05-29T09:23:48Z

src/plotting.jl

-                t,  y[i, :]
+                label   --> (multiseries > 1 ? "From u($(ms))" : "")
+                subplot --> i#s2i(1,i)
+                t,  r.y[i, :, ms]


Your suggestion of PermuteDims in #480 didn't make it in? It seemed like the right choice for consistency, but would perhaps not work so well here?

If possible, I would be happy with a more descriptive name than r, some suggestions: res. simres, simdata

olof3 · 2021-05-29T09:24:16Z

src/timeresp.jl

        y = Array{T}(undef, ny, length(t), nu)
        for i=1:nu
-            y[:,:,i], t, x[:,:,i],_ = lsim(sys[:,i], u, t; x0=x0s[:,i], method=method, kwargs...)
+            y[:,:,i], t, x[:,:,i],uout = lsim(sys[:,i], u, t; x0=x0s[:,i], kwargs...)


Suggested change

y[:,:,i], t, x[:,:,i],uout = lsim(sys[:,i], u, t; x0=x0s[:,i], kwargs...)

y[:,:,i], t, x[:,:,i], uout = lsim(sys[:,i], u, t; x0=x0s[:,i], kwargs...)

olof3 · 2021-05-29T09:24:27Z

src/timeresp.jl

    end
    if nu == 1 # Why two cases # QUESTION: Not type stable?
-        y, t, x,_ = lsim(sys, u, t; x0=x0s[:], method=method, kwargs...)
+        y, t, x,uout = lsim(sys, u, t; x0=x0s[:], kwargs...)


Suggested change

y, t, x,uout = lsim(sys, u, t; x0=x0s[:], kwargs...)

y, t, x, uout = lsim(sys, u, t; x0=x0s[:], kwargs...)

baggepinnen · 2021-05-30T09:44:14Z

One could potentially store a metadata field in the struct that contains additional information regarding how the result was produced.

In the simplest case, this could be a string or a symbol, e.g., metadata = :step to indicate that the result was produced using the function step. Even more advanced and flexible use could be to store a Dict that can contain arbitrary information, but I'm not sure what else one would like to store.

baggepinnen · 2021-08-21T05:15:26Z

I'm more or less done with this PR. Additional metadata can be added at a later point should someone feel compelled to add it. If there are no objections, I will model the freqresp return structure similarly in a new PR when this has been merged

* Avoid unnecessarily large realization for feedback of TransferFunction (#485) * Avoid unnecessarily large realization for feedback of TransferFunction * Fix and added more tests. * Change to numpoly, denpoly * add dev brach to PR CI * Switch u layout (#480) * switch u layout for lsim * Update src/timeresp.jl Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com> * More updates, one error in test_timeresp * Fix tests * Change to AbstractVecOrMat * Catch CuArray in matrix conversion * General zero vectors for x0 to support GPUs * Update src/timeresp.jl Co-authored-by: Mattias Fält <mfalt@users.noreply.github.com> * Update src/timeresp.jl Co-authored-by: Mattias Fält <mfalt@users.noreply.github.com> * Move f outside lsim * Remove GPU compatible x0, save for later * Fix doctest * add kwargs * Remove variable and generalize type Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com> Co-authored-by: Mattias Fält <mfalt@users.noreply.github.com> * Gangof4 fixes (#486) * QOL improvements for plotting * remove spurious getindex * update docstring * multiple Ms in nyquist * bugfixes * make rings appear in all subplots * Minor fixes to gang-of-four functionality. * Fixes to Nyquist plots. * Updated the nyquistplot docstring Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com> * add frequency in Hz to dampreport (#488) * updates to nyquistplot (#493) * updates to nyquistplot * Update src/plotting.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update src/plotting.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: olof3 <olof3@users.noreply.github.com> * fix traces in rlocus (#491) * fix traces in rlocus * Update src/pid_design.jl Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se> Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se> * let lsim handle arguments in lsimplot (#492) * bugfix: avoid creating continuous system with Ts=0.0 (#496) * Deactivate _preprocess_for_freqresp (#497) until hessenberg is properly used * allow balance when converting tf to ss (#495) * allow balance when converting tf to ss * use zeros(T) instead of fill(zero(T)) * Update src/types/StateSpace.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update src/types/conversion.jl Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se> Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se> * Better handling of problematic cases for delay systems (#482) * delay error should be correct now * warn limit * Add tustin c2d/d2c method (#487) * add Tustin discretization * no indexing after c2d * implement f_prewarp in tustin and test vs. matlab * fixe use wrong Ts * f_prewarp -> w_prewarp * w_prewarp in tests * correct handling of x0 in lsimplot (#498) * move eye def to framework.jl (#499) * Fix UndefVarError: T not defined (#501) * add axes for ss (#504) * pi place and tests (#502) * pi place and tests * Fix test * Add ending space in file * Update numvec denvec * Fixed error * Update test/test_pid_design.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update test/test_pid_design.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update forms * Fix test * Fix test * Update src/pid_design.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update src/pid_design.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update src/pid_design.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: olof3 <olof3@users.noreply.github.com> * Conver to tf in placepi (#507) * Use nstates instead of nx in lsimplot (#508) * Use nstates instead of nx in lsimplot * Add predictor (#511) * up innovation_form and add noise_model * keep innovation_form, add predictor * export predictor * add hats * updates to `obsv` (#517) * updates to `obsv` All computing an arbitrary number of rows in the observability matrix and accept `AbstractStateSpace` * Update src/matrix_comps.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: olof3 <olof3@users.noreply.github.com> * `hz` keyword in `nyquistplot` similar to in `bodeplot` (#518) * Fixes to place (#500) * Add tests for place. * Removed luenberger and exented place instead. Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com> * allow AbstractStateSpace in several places (#520) * allow AbstractStateSpace in several places * Maintain type for zpk in c2d (#522) * maintain zpk type in c2d * Fix type in typeconversion for c2d tf * Fix type in c2d tf/zpk * Remove assertion on tf/zpk SISO for c2d * Test for c2d(zpk(..)..) unbroken * Keep MIMO assertion c2d tf * Add comment and fix test * Fix test * h to Ts * Fix test * remove assert * split into methods * Remove asserts (#523) * Replace asserts * add error types * fix conversion for custom types (#514) * fix conversion for custom types * special numeric_type for AbstractSS * fix conversion from ss to tf without type * more abstract statespaces (#521) * omre abstract statespaces * even more ASS * remove simple feedback in favor of mor egeneral version * propagate timeevol * add block diagram to feedback docstring * Updates to docstring * fix docstring formatting * delete redundancy in feedback docstring Co-authored-by: olof3 <olof3@users.noreply.github.com> * add controller function (#512) * add controller function * rename controller and predictor * Move plot globals to runtests (#531) * Move plot globals to runtests * Move plot globals to framework.jl * return similarity transform from `balreal` (#530) * display error when covar fails * return similarity transform from balreal * Update src/matrix_comps.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: olof3 <olof3@users.noreply.github.com> * remove incorrect warning in pzmap (#535) * support hz keyword in sigmaplot similar to bodeplot (#537) * change formatting in dampreport (#536) * change formatting in dampreport * update dampreport to use ± * even better formatting * up formatting for complex systems * add additive identity element for statespace and TF (#538) * add additive identity element for statespace and TF * rm zero from type. Test MIMO zero * Fix struct_ctrb_obsv (#540) * Fix struct_ctrb_obsv, closes #409 * Update src/simplification.jl Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com> * add to docstring Co-authored-by: olof3 <olof3@users.noreply.github.com> * Yet another fix for struct_ctrb_states. Closes #475 (#541) * bugfix for negative real pole in damp (#539) * Fix #546 * minor typographic changes * optional epsilon in dcgain (#548) * optional epsilon in dcgain * Update analysis.jl * bugfix: use correct type for saving dde solutions (#549) Would be good to have a regression test for `BigFloat` data which was the reason I found this bug. That seems slightly involved to fix though. I assume that this would also have failed for `ComplexF64`, so eventually a test for that too would be good. * bugfix dcgain (#551) * correct type of initial state in step (#553) * remove version checks * Fix spacing in type printing * write `struct_ctrb_states` in terms of `iszero` instead of `== 0` (#557) * write `struct_ctrb_states` in terms of `iszero` instead of `== 0` The reason is that `iszero` always returns a bool, whereas `== 0` may return anything. The difference appears for symbolic variables where ```julia julia> q0 == 0 q0(t) == 0 julia> iszero(q0) false ``` * Update simplification.jl * remove `issmooth` (#561) * remove issmooth * drop extra `]` * Return a result structure from lsim (#529) * Return a result structure from lsim This is by design a non-breaking change to the lsim interface since the structure allows both getindex and destructuring to behave just like if lsim returned a tuple of arrays like before. Indeed, the tests for lsim were not touched in this commit. This commit also adds a plot recipe to the result structure. All plot recipes for lsimplot, stepplot, impulseplot have been replaced by the new recipe. This is a breaking change since the names of the previous plots no longer exist. A slight change is that the plots for a step response no longer show the text "step response", but I think that's an acceptable change, the user can supply any title they prefer themselves. * remove automatic title * introduce additional abstract result type * do not destructure sys * remove LQG (#564) The functionality was very buggy and poorly tested. A much improved version with proper tests are available in https://github.com/JuliaControl/RobustAndOptimalControl.jl/blob/master/src/lqg.jl * pole->poles tzero->tzeros (#562) * update usage of plot for step simulation * add doctest filters * add release notes * Update README.md * Update README.md Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se> Co-authored-by: Mattias Fält <mfalt@users.noreply.github.com>

olof3 reviewed May 29, 2021

View reviewed changes

baggepinnen added 3 commits May 30, 2021 11:48

remove automatic title

8a4d560

introduce additional abstract result type

276db69

do not destructure sys

b59bb36

murrayrm mentioned this pull request Aug 26, 2021

Time response data class python-control/python-control#649

Merged

baggepinnen added 2 commits October 20, 2021 11:46

Merge branch 'dev' into simresult

5c00751

Merge branch 'dev' into simresult

0088980

baggepinnen merged commit 851ecbc into dev Oct 20, 2021

baggepinnen deleted the simresult branch October 20, 2021 12:18

baggepinnen added the v1 Issues to resolve before releasing v1 label Oct 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return a result structure from lsim#529

Return a result structure from lsim#529
baggepinnen merged 6 commits intodevfrom
simresult

baggepinnen commented May 29, 2021 •

edited

Loading

Uh oh!

JuliaControlBot commented May 29, 2021

Uh oh!

JuliaControlBot commented May 29, 2021

Uh oh!

codecov bot commented May 29, 2021 •

edited

Loading

Uh oh!

olof3 left a comment

Uh oh!

olof3 May 29, 2021

Uh oh!

olof3 May 29, 2021 •

edited

Loading

Uh oh!

olof3 May 29, 2021

Uh oh!

olof3 May 29, 2021

Uh oh!

baggepinnen commented May 30, 2021

Uh oh!

baggepinnen commented Aug 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	y[:,:,i], t, x[:,:,i],uout = lsim(sys[:,i], u, t; x0=x0s[:,i], kwargs...)
	y[:,:,i], t, x[:,:,i], uout = lsim(sys[:,i], u, t; x0=x0s[:,i], kwargs...)

	y, t, x,uout = lsim(sys, u, t; x0=x0s[:], kwargs...)
	y, t, x, uout = lsim(sys, u, t; x0=x0s[:], kwargs...)

Conversation

baggepinnen commented May 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JuliaControlBot commented May 29, 2021

Uh oh!

JuliaControlBot commented May 29, 2021

Uh oh!

codecov bot commented May 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

olof3 left a comment

Choose a reason for hiding this comment

Uh oh!

olof3 May 29, 2021

Choose a reason for hiding this comment

Uh oh!

olof3 May 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olof3 May 29, 2021

Choose a reason for hiding this comment

Uh oh!

olof3 May 29, 2021

Choose a reason for hiding this comment

Uh oh!

baggepinnen commented May 30, 2021

Uh oh!

baggepinnen commented Aug 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

baggepinnen commented May 29, 2021 •

edited

Loading

codecov bot commented May 29, 2021 •

edited

Loading

olof3 May 29, 2021 •

edited

Loading