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.

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

Add constructors with nu, ny, nx for easy rebuilding #399

Closed
jonniedie opened this issue Nov 23, 2020 · 3 comments · Fixed by #624
Closed

Add constructors with nu, ny, nx for easy rebuilding #399

jonniedie opened this issue Nov 23, 2020 · 3 comments · Fixed by #624

Comments

@jonniedie
Copy link
Contributor

jonniedie commented Nov 23, 2020

Some useful functions that need to recurse through arbitrarily nested structs and rebuild them with different values--like @set and @set! from Setfield.jl or replace_particles (along with its derived functions mean_object and nominal) from MonteCarloMeasurements.jl--require constructors that match the positions of the type's fields. Since TransferFunction and StateSpace have fields for their number of inputs/outputs[/states], but no constructors that take them in, it creates problems for these types of functions. For example, these don't currently work:

julia> using ControlSystems, MonteCarloMeasurements, Setfield

julia> a = zpk([-1±0.1], [im, -im], 5);

julia> @set! a.matrix[1].k = 8
ERROR: MethodError: no method matching TransferFunction(::Array{ControlSystems.SisoZpk{Particles{Float64,2000},Complex{Particles{Float64,2000}}},2}, ::Continuous, ::Int64, ::Int64)
...

julia> mean_object(a)
┌ Error: Failed to create a `TransferFunction` by calling it with its fields in order. For this to work, `TransferFunction` must have a constructor that accepts all fields in the order they appear in the struct and 
accept that the fields that contained particles are replaced by 0. Try defining a meaningful constructor that accepts arguments with the type signature
...

but adding

StateSpace(A, B, C, D, timeevol, nx, nu, ny) = StateSpace(A, B, C, D, timeevol)

makes it so they work:

julia> @set! a.matrix[1].k = 8
TransferFunction{Continuous,ControlSystems.SisoZpk{Particles{Float64,2000},Complex{Particles{Float64,2000}}}}
   1.0s + 1.0 ± 0.1
8.0----------------
     1.0s^2 + 1.0

Continuous-time transfer function model

julia> mean_object(a)
TransferFunction{Continuous,ControlSystems.SisoZpk{Float64,Complex{Float64}}}
    1.0s + 1.0
8.0------------
   1.0s^2 + 1.0

Continuous-time transfer function model

This and the similar method for TransferFunction, are all that's needed for these types of functions to work. I can make a pull request with these, if it would be useful.

@jonniedie jonniedie changed the title Constructors with nu, ny, nx for easy rebuilding Add constructors with nu, ny, nx for easy rebuilding Nov 23, 2020
jonniedie added a commit to jonniedie/ControlSystems.jl that referenced this issue Nov 23, 2020
@baggepinnen
Copy link
Member

I think that these fields were added as a convenience back in the day when one could not overload getproperty. I think it would be safe to add them to getproperty and remove the fields? @mfalt can you see any reason to keep them around, other than the nice way of accessing the sizes?

@olof3
Copy link
Contributor

olof3 commented Nov 23, 2020

I think that these fields were added as a convenience back in the day when one could not overload getproperty. I think it would be safe to add them to getproperty and remove the fields?

I absolutely agree, I have been thinking this a couple of times as well

baggepinnen added a commit to baggepinnen/ControlSystems.jl that referenced this issue Dec 12, 2020
baggepinnen added a commit to baggepinnen/ControlSystems.jl that referenced this issue Dec 12, 2020
baggepinnen added a commit that referenced this issue Jan 17, 2021
* Solve #399

* temp. deactivate test that fails on v1.5 only

* temp. deactivate test that fails on v1.5 only
@baggepinnen
Copy link
Member

This should be solved for statespace types now since we removed the internal fields that were no longer required. Might still be a problem for transfer functions though.

baggepinnen added a commit that referenced this issue Jan 28, 2022
baggepinnen added a commit that referenced this issue Jan 28, 2022
baggepinnen added a commit that referenced this issue Jan 29, 2022
* Closes #399

* handle propertynames properly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants