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

Changing to the propper "Plots.jl" names to avoid warnings #316

Merged
merged 3 commits into from Sep 23, 2020

Conversation

KronosTheLate
Copy link
Contributor

@KronosTheLate KronosTheLate commented Sep 16, 2020

So I have only changed "color" into "seriescolor" and "x/ylabel" into "x/yguide" where I could find them, inside the call to the @series macros. This is my first pull-request and commit, and so any feedback is appreciated. I don't really know what I am doing, but I hope and believe that I fixed it right ^_^

First commit, so I may have done something wrong. Feedback is appreciated.
I changed ylabel, ylabel and color to yguide, xguide and seriescolor in accordance with the error message I got from doing an impulse-plot, copied below. I also changed color to seriescolor for the lsimplot function. This is done to avoid unnecessary warnings:
```julia
Warning: Attribute alias `color` 
detected in the user recipe defined for the signature (::ControlSystems.Impulseplot). To ensure expected 
behavior it is recommended to use the default attribute `seriescolor`.
└ @ Plots C:\Users\user\.julia\packages\Plots\ViMfq\src\pipeline.jl:15
┌ Warning: Attribute alias `ylabel` detected in the user recipe defined for the signature (::ControlSystems.Impulseplot). To ensure expected behavior it is recommended to use 
the default attribute `yguide`.    
└ @ Plots C:\Users\user\.julia\packages\Plots\ViMfq\src\pipeline.jl:15
┌ Warning: Attribute alias `xlabel` detected in the user recipe defined for the signature (::ControlSystems.Impulseplot). To ensure expected behavior it is recommended to use 
the default attribute `xguide`.    
└ @ Plots C:\Users\user\.julia\packages\Plots\ViMfq\src\pipeline.jl:15
```
@baggepinnen
Copy link
Member

Nice, thanks :)
Can you please verify that you're able to change the color of, e.g., a bodeplot by using the keywords c, color? Like so

bodeplot(sys, c=:blue)

We've had problems with this before, that depending on what keyword we used in the plot recipe the results would be different when the user provided these magic keywords c, color etc.

@coveralls
Copy link

coveralls commented Sep 16, 2020

Coverage Status

Coverage increased (+0.07%) to 57.405% when pulling 1a5c7b7 on KronosTheLate:master into 47151aa on JuliaControl:master.

@KronosTheLate
Copy link
Contributor Author

I copy-pasted the file I edited in the PR into my own package (Is that a legit move? It seems to work at least, ehe...), and ran both bodeplot and impulseplot with keywords c and color, everything works fine. No errors and colors as expected

However, when I run this code

L = 1.5
R = 1
C = 0.8
num = [1.0]
den = [L*C, R*C, 1]
TF = tf(num, den)
#mag, phase, w = bode(TF)
bodeplot(TF, color=:red)

I get 456 lines printed into REPL (in VSCode) saying ERROR: syntax error. But the plot looks great :D Maybe I should open a separate issue for that?

A third, unrelated thing: how do I know what impulse my model is subjected to in an impulseplot, and how do I edit it? I saw nothing on it in the docs for impulseplot, it only mentions the final time Tf and discretization time Ts

@baggepinnen
Copy link
Member

Alright great! The ERROR: syntax error appears with the GR backend and I think it's due to how we provide latex strings. It's a hassle to get rid of because the behavior of GR has changed so many times in the past that every time we fix it it breaks a few months later :(

The impulse is such that it has unit area but the shortest possible time. For discrete systems this means one sample period, for continuous systems this means a Dirac delta function. You can not change this, but you can provide arbitrary inputs to the function lsim. In practice, it will be hard to simulate "almost" impulses for continuous time systems. The current simulation relies on mathematical properties of a true Dirac impulse to perform the simulation. See

function impulse(sys::StateSpace, t::AbstractVector; method=:cont)
where the system is rewritten and a special initial condition is set

@KronosTheLate
Copy link
Contributor Author

Alright great! The ERROR: syntax error appears with the GR backend and I think it's due to how we provide latex strings. It's a hassle to get rid of because the behavior of GR has changed so many times in the past that every time we fix it it breaks a few months later :(

That is a shame, the LaTeX looks great. With the ployly backend it is just un-parsed latex, so that is also not great.

The impulse is such that it has unit area but the shortest possible time. For discrete systems this means one sample period, for continuous systems this means a Dirac delta function. You can not change this, but you can provide arbitrary inputs to the function lsim.

I think I will add this to the doc-string, if you are okay with me using your words? Ofc, you will probably have the final say on it anyways, so you can be a filter if I write something stupid/inaccurate. I will try to say no more than you did.

In practice, it will be hard to simulate "almost" impulses for continuous time systems. The current simulation relies on mathematical properties of a true Dirac impulse to perform the simulation. See

function impulse(sys::StateSpace, t::AbstractVector; method=:cont)

where the system is rewritten and a special initial condition is set

I have to admit, there is a lot I don't understand. I am a third semester engineering student who happens to prefer Julia to MatLab and Simulink if I can (due to cost, and Julia syntax/comunity/speed are all so great), so I am checking out alternatives. I had pretty much no idea what a control-system, a transfer-function or the laplace-domain were a few short weeks ago (I still don't really KNOW what the laplace-domain is, but it sure is nice for math!). I think I have to learn more before I can take full use of your package, which really looks like it can do a lot of nice things. Good job, and thank you for your nice and full answers 😄

(Is this where I close the issue?)

@baggepinnen
Copy link
Member

baggepinnen commented Sep 16, 2020

(Is this where I close the issue?)

Please don't close, it's a PR ;)

I'll merge it once all tests have passed.

And yes, feel free to add to the docstring of impulse using any version of my formulations.

@baggepinnen baggepinnen merged commit 6599e13 into JuliaControl:master Sep 23, 2020
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 this pull request may close these issues.

None yet

3 participants