Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid unnecessarily large realization for feedback of TransferFunction #485

Merged
merged 3 commits into from
May 12, 2021
Merged

Avoid unnecessarily large realization for feedback of TransferFunction #485

merged 3 commits into from
May 12, 2021

Conversation

olof3
Copy link
Contributor

@olof3 olof3 commented May 12, 2021

No description provided.

@JuliaControlBot
Copy link

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 Reference New
✔️ 0.001 Reference New

baggepinnen
baggepinnen reviewed May 12, 2021
View reviewed changes
src/connections.jl Outdated Show resolved Hide resolved
@albheim
Copy link
Member

albheim commented May 12, 2021

Looks good, this would solve the symbolics problem I assume?

@baggepinnen baggepinnen merged commit ccab858 into dev May 12, 2021
@baggepinnen baggepinnen deleted the fix_feedback_tf branch May 12, 2021 12:32
@baggepinnen
Copy link
Member

Nice, this type of improvements make the transfer-function misery slightly less miserable :)

@olof3
Copy link
Contributor Author

olof3 commented May 12, 2021

The tests didn't seem to run on any commits except the first one (where it failed)?

Looks good, this would solve the symbolics problem I assume?

Yes, now it works :)

@baggepinnen
Copy link
Member

ah, sneaky green checkmark then. If we're going to pursue this dev branch strategy we need to find a way to make CI behave as for the master branch, perhaps we can figure out how others do it.

@albheim
Copy link
Member

albheim commented May 12, 2021

I think we could just add it to the test.yml

name: CI 

on:
  pull_request:
    branches:
      - master
      - dev          <--- this
  push:
    branches:
      - master
    tags: 
      - '*'

jobs:
  runtests: 
...

baggepinnen added a commit that referenced this pull request Nov 7, 2021
@baggepinnen @olof3
@albheim @mfalt
Releasenotes (#566)
30bf61a 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baggepinnen baggepinnen baggepinnen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@olof3 @JuliaControlBot @albheim @baggepinnen