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

let lsim handle arguments in lsimplot #492

Merged
merged 1 commit into from
May 13, 2021
Merged

let lsim handle arguments in lsimplot #492

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 5 additions & 14 deletions src/plotting.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,30 +116,21 @@ end
@userplot Lsimplot

"""
fig = lsimplot(sys::LTISystem, u, t; x0=0, method)
lsimplot(LTISystem[sys1, sys2...], u, t; x0, method)
fig = lsimplot(sys::LTISystem, u, t; x0=0)
lsimplot(LTISystem[sys1, sys2...], u, t; x0)

Calculate the time response of the `LTISystem`(s) to input `u`. If `x0` is
not specified, a zero vector is used.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the impulse and step plots they claim that kwargs are sent to plot, though I can't really see how that is achieved. Just thinking if that is also done for this?

Also could add the See also [lsim](@ref) which is similarly found in impulse and step to reference where to find more information about the options.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kwargs are automatically sent to the plots pipeline since lsimplot is implemented as a recipe. Any kwarg not intercepted by the function signature will be forwarded to the rest of the plot pipeline.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, cool. But then it seems like you don't intercept any kwargs as it is now in lsimplot? And it calls y, t = lsim(s, p.args[2:end]...) which seems to only use the args?

Something still looks a bit weird to me, but I guess that is not introduced with this PR cause the x0 was the same before. Did a quick test and it seems like x0 is not expected to be a kwarg in the currect version. So either docstring should reflect this or we need to catch the kwargs and forward them.

  • lsimplot(sys, u, t; x0=x0) does not work
  • lsimplot(sys, u, t, x0=x0) does not work (as expected)
  • lsimplot(sys, u, t, x0) does work

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean now, the docstring has a semicolon where it should have a colon. lsimplot(sys, u, t, x0) is the correct syntax since it aligns with lsim. Feel free to submit PR, otherwise I'll make the change in a few hours

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, lsim actually has x0 as a keyword arg, I seem to have confused myself a bit...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the lsim with x0 in the args seems to be deprecated so we probably should avoid using that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a fix upcoming. Keyword-arg slurping can unfrtunately not be used in recipices, so all keyword args that are to be utilized by anything other than plotting must be handled manually, which is a bit of a pain.


Continuous time systems are discretized before simulation. By default, the
method is chosen based on the smoothness of the input signal. Optionally, the
`method` parameter can be specified as either `:zoh` or `:foh`.
"""
lsimplot

@recipe function lsimplot(p::Lsimplot; method=nothing)
if length(p.args) < 3
error("Wrong number of arguments")
end
systems,u,t = p.args[1:3]
@recipe function lsimplot(p::Lsimplot)

systems = p.args[1]

if !isa(systems,AbstractArray)
systems = [systems]
end
if method == nothing
method = _issmooth(u) ? :foh : :zoh
end
if !_same_io_dims(systems...)
error("All systems must have the same input/output dimensions")
end
Expand Down