Switch u layout #480

albheim · 2021-05-07T14:37:52Z

Switch the layout for u in the toolbox.

JuliaControlBot · 2021-05-07T14:46:20Z

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

baggepinnen

Looks good. Would be nice to also have a method that accepts preallocated x, but can be a separate PR. No tests need tweaking to accomodate the changes?

src/timeresp.jl

baggepinnen · 2021-05-07T19:02:43Z

src/timeresp.jl

-        for (i,ti) in enumerate(t)
-            uout[i,:] = u(x[i,:],ti)'
-        end
+        f(dx,x,p,t) = dx .= sys.A*x .+ sys.B*u(x,t)


Here we have an opportunity to do better in case arrays are static and we take up the direct dependence on StaticArrays. The dx .= will fail for static arrays.

I haven't used static arrays at all, so not familiar with why that would fail. What would be the static array friendly option?

A static array is static, hence trying to mutate it with dx .= something is not allowed. A static-arrays friendly version would not use any mutation and just write the code as if it allocated, because static arrays are stack-allocated and hence the allocation is free. This code is still correct from a regular array perspective, so any optimizations for static arrays would be in form of another method, i.e.,

f(x::SArray,p,t) = sys.A*x .+ sys.B*u(x,t)

Ahh, thought you only meant that A and B should be static. But everything is going to be static. Sounds interesting, will have to have a look at that.

Should it then be up to the user to define x0 and then the simulation is done with that array type for x?

albheim · 2021-05-07T20:55:00Z

Looks good. Would be nice to also have a method that accepts preallocated x, but can be a separate PR. No tests need tweaking to accomodate the changes?

Yeah, I think there might be some test (along with maybe other dependent methods) to fix before this is ready. Just started it and used it to check if I could push. Will fix the rest when I have time, but that will likely be after the weekend. But you are very welcome to have a look at it if you want.

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

src/timeresp.jl

albheim · 2021-05-08T10:03:46Z

Had some time this morning so tried to go through a bit more, think most dependent methods of lsim and tests are now switched. One error outside of test when running tests, and haven't had time to find it yet. Need to head out now so thought I would push what I had.

src/timeresp.jl

Co-authored-by: Mattias Fält <mfalt@users.noreply.github.com>

…trolSystems.jl into switch_u_layout

albheim · 2021-05-11T20:43:40Z

Seems like all tests and doctests are passing now. Good to merge?

mfalt · 2021-05-11T22:27:52Z

@JuliaControlBot test-plots

JuliaControlBot · 2021-05-11T22:36:46Z

This is an automated message.
Plots were compared to references. 2/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
✔️ 0.0
✔️ 0.001

mfalt · 2021-05-11T22:58:55Z

Tests look good, feel free to merge.

olof3 · 2021-05-12T01:16:08Z

src/timeresp.jl

+        x = Array{Float64}(undef, nx, lt, nu)
+        y = Array{Float64}(undef, ny, lt, nu)


Hmm. Is this how we want it?

I guess this is the best way wrt memory layout, but it does seem quite confusing and inconsistent with the order that would be natural for e.g., freqfresp, where we would have the frequency index last.

I thought this is what we said for the first two arguments, and the last one made sense to me to leave based on how it was accessed. What would the other alternative be, nx x nu x lt?

Currently freqresp has the frequency index in the first dimension if I'm not mistaken, why would it be the most natural to have it last?

Also, Float64 should probably be made generic?

freqresp probably needs a change corresponding to the one in this PR to place frequency in the last dimension, since a single call to evalfr generates a matrix ny x nu of frequency responses at a single frequency.

One can always use PermutedDimsArray in the end to shape the array the way we want without copying. The user can then, if they choose, call copy on it to obtain a regular array, or use the PermutedDimsArray like it was a regular array

julia> PermutedDimsArray(randn(1,2,3), (2,3,1)) 2×3×1 PermutedDimsArray(::Array{Float64, 3}, (2, 3, 1)) with eltype Float64:

PermutedDimsArray seems like a reasonable fix.

Would probably be better to avoid introducing lt, consider just using length(t) or possibly call it nt instead.

Also, Float64 should probably be made generic?

Seem reasonable, copied the way used in impulse.

Would probably be better to avoid introducing lt, consider just using length(t) or possibly call it nt instead.

I switched to using length(t) instead.

codecov · 2021-05-12T13:24:57Z

Codecov Report

Merging #480 (1c4ef6e) into dev (17e78c1) will decrease coverage by 0.03%.
The diff coverage is 84.44%.

@@            Coverage Diff             @@
##              dev     #480      +/-   ##
==========================================
- Coverage   85.02%   84.99%   -0.04%     
==========================================
  Files          31       31              
  Lines        3119     3125       +6     
==========================================
+ Hits         2652     2656       +4     
- Misses        467      469       +2

Impacted Files	Coverage Δ
src/synthesis.jl	`65.11% <ø> (ø)`
src/delay_systems.jl	`89.61% <60.00%> (ø)`
src/timeresp.jl	`92.42% <86.48%> (-0.17%)`	⬇️
src/plotting.jl	`79.81% <100.00%> (ø)`
src/utilities.jl	`83.67% <100.00%> (ø)`
src/connections.jl	`93.47% <0.00%> (-0.81%)`	⬇️

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 17e78c1...1c4ef6e. Read the comment docs.

* 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>

switch u layout for lsim

e1bc88f

baggepinnen reviewed May 7, 2021

View reviewed changes

albheim and others added 2 commits May 7, 2021 22:55

Update src/timeresp.jl

9c23ecf

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

More updates, one error in test_timeresp

7c80632

albheim commented May 8, 2021

View reviewed changes

src/timeresp.jl Outdated Show resolved Hide resolved

albheim and others added 4 commits May 9, 2021 20:43

Fix tests

47264cf

Change to AbstractVecOrMat

ad98b33

Catch CuArray in matrix conversion

4c2566d

General zero vectors for x0 to support GPUs

f4eef18

mfalt reviewed May 11, 2021

View reviewed changes

src/timeresp.jl Outdated Show resolved Hide resolved

mfalt reviewed May 11, 2021

View reviewed changes

src/timeresp.jl Outdated Show resolved Hide resolved

mfalt reviewed May 11, 2021

View reviewed changes

src/timeresp.jl Outdated Show resolved Hide resolved

albheim and others added 6 commits May 11, 2021 11:29

Update src/timeresp.jl

dfa7918

Co-authored-by: Mattias Fält <mfalt@users.noreply.github.com>

Update src/timeresp.jl

a77de03

Co-authored-by: Mattias Fält <mfalt@users.noreply.github.com>

Move f outside lsim

ef24346

Merge branch 'switch_u_layout' of https://github.com/JuliaControl/Con…

a238dc7

…trolSystems.jl into switch_u_layout

Remove GPU compatible x0, save for later

9dc5097

Fix doctest

ebab7f4

olof3 reviewed May 12, 2021

View reviewed changes

add kwargs

9236684

Remove variable and generalize type

1c4ef6e

albheim merged commit 64f8214 into dev May 12, 2021

albheim deleted the switch_u_layout branch May 12, 2021 16:07

This was referenced May 13, 2021

Lsim gpu #468

Closed

Input/output format of lsim #469

Closed

olof3 mentioned this pull request May 29, 2021

Return a result structure from lsim #529

Merged

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

Switch u layout #480

Switch u layout #480

albheim commented May 7, 2021

JuliaControlBot commented May 7, 2021

baggepinnen left a comment

baggepinnen May 7, 2021

albheim May 9, 2021

baggepinnen May 12, 2021

albheim May 12, 2021

albheim commented May 7, 2021

albheim commented May 8, 2021 •

edited

Loading

albheim commented May 11, 2021

mfalt commented May 11, 2021

JuliaControlBot commented May 11, 2021

mfalt commented May 11, 2021

olof3 May 12, 2021

albheim May 12, 2021

baggepinnen May 12, 2021

baggepinnen May 12, 2021

baggepinnen May 12, 2021

olof3 May 12, 2021

olof3 May 12, 2021

albheim May 12, 2021

codecov bot commented May 12, 2021 •

edited

Loading

		x = Array{Float64}(undef, nx, lt, nu)
		y = Array{Float64}(undef, ny, lt, nu)

Switch u layout #480

Switch u layout #480

Conversation

albheim commented May 7, 2021

JuliaControlBot commented May 7, 2021

baggepinnen left a 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

Choose a reason for hiding this comment

albheim commented May 7, 2021

albheim commented May 8, 2021 • edited Loading

albheim commented May 11, 2021

mfalt commented May 11, 2021

JuliaControlBot commented May 11, 2021

mfalt commented May 11, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 12, 2021 • edited Loading

Codecov Report

albheim commented May 8, 2021 •

edited

Loading

codecov bot commented May 12, 2021 •

edited

Loading