Improve documentation for tf(), step, stepplot, impulse and impulseplot
In this PR I have changed the following
- changed the documentation for "creating systems" to include an example of using
s=tf("s")syntax - included the julia> text consistently in the examples under "creating systems"
- removed the #output comment under "creating systems" as I found it un-nessecary, and I don't like the fact that it is not copy-pasted from the REPL, which I think examples should be when possible for maximum clarity
- Clearly stated in the documentation for step, stepplot, impulse and impulseplot what they do
- Fixed a typo in the impulseplot documentation where it said "step" instead of "impulse"
In regards to what you said @baggepinnen :
@KronosTheLate, don't forget to base PR:s off the updated master branch of the base package, i.e., pull ControlSystems/master into master on your fork. This way only the commits that are new appear in the PR, and not the commits from your previous PR.
I tried to google how to in multiple different ways, and I also tried to update my personal branch (PR 7), before finally seeing that I could just base this PR off of master. But this PR IS in fact based off of master, just like the last one: "KronosTheLate wants to merge 11 commits into JuliaControl:master from KronosTheLate:master"
I am not sure how to only make this PR for the new changes, but have simply been wanting to fix this for a week, and I don't want to prioritize finding out how to do it properly, and having it on my mind for more days. I set aside some time now, and I used close to half of it trying to follow your advice. I apologize for my GitHub-illiteracy, I hope you are able to squash it nicely (if you like the changes), and I am very open to step-by-step instructions on how to do it right, or a link to an easy to understand resource.
Coverage decreased (-2.1%) to 55.538% when pulling 0b6ca0c6c23eb437a7894c64b4f1e9953697bfd6 on KronosTheLate:master into 0f3c31376866d7f68070a43eadf59cd7f8623f76 on JuliaControl:master.
Coverage remained the same at 57.791% when pulling ad9a4abcca1a8092b6d5ee7c3c77d1ea8b196114 on KronosTheLate:master into e993f7be4d287d1b517b02933f985d99ea5cf4b7 on JuliaControl:master.
@olof3 and @baggepinnen thank you so much for the thorough feedback, much appriciated. First time adding real documentation, so pointers are very much appriciated. Ill look through it properly tomorrow, but from what I can tell now I will add all the suggestions 😃
Looks good. I just want to double check with @mfalt so that the
#outputsyntax is not needed for Documenter.jl doctests?
I don't think those are working properly currently anyway. When I have time to fix https://github.com/JuliaControl/ControlSystems.jl/pull/293 I'll check if I need to re-add them, but I see no need to delay this PR.
I think some of the wording could be improved, but I guess it is better to have some documentation than none.
Edit: it is possible that the docs are not automatically building currently. I will look into it
@KronosTheLate it is great to see that you want to improve the docs. There is a lot missing, and it is great with input from new users. Most of the maintainers here are more or less PhDs in control, so there is always a risk that we write the documentation in a way that is hard to understand for new users. However, it is also important that the documentation is fairly accurate, so I really appreciate that you are open to discussion and input.
I will try to add and rewrite significant parts of the documentation of the package, over the weekend if I find the time. Any input on the new documentation would be very helpful. The changes in this PR will of course also be pulled in some way.
@KronosTheLate it is great to see that you want to improve the docs. There is a lot missing, and it is great with input from new users. Most of the maintainers here are more or less PhDs in control, so there is always a risk that we write the documentation in a way that is hard to understand for new users. However, it is also important that the documentation is fairly accurate, so I really appreciate that you are open to discussion and input.
I will try to add and rewrite significant parts of the documentation of the package, over the weekend if I find the time. Any input on the new documentation would be very helpful. The changes in this PR will of course also be pulled in some way.
I appreciate being heard by people who know a lot more than me ^_^ I have to make clear, I did not know what a transfer-function was a couple of months ago. I am just going to start looking at actually preforming any control of a system (we have only looked at closed loops without a propper actuator, I believe). I am SO green on this subject. But I am also psyched that there is a Julia-package than can do basically everything we are doing in MatLab, allowing independence from the MatLab license. I am so grateful for the work you guys have put into this, making Julia a viable option :D
I will be honoured to make a can-I-understand-this review on your improvements to the docs. I am scared that I might not know what we are talking about at some point, not because of the phrasing, but because I don't have a clue. And as the docs are not to educate people on relevant theory, I might be clueless at some points. But I'll do my best ^_^
I have struggled a little with taking in reviews, having them become outdated, and some conflicts. But I think that it should be all good now, so as far as I know, this PR is ready for merge.
Codecov Report
Merging #344 (78d68cb) into master (e9837ee) will decrease coverage by
0.79%. The diff coverage isn/a.
@@ Coverage Diff @@
## master #344 +/- ##
==========================================
- Coverage 82.40% 81.60% -0.80%
==========================================
Files 31 31
Lines 2824 2827 +3
==========================================
- Hits 2327 2307 -20
- Misses 497 520 +23
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/plotting.jl | 78.79% <ø> (ø) |
|
| src/timeresp.jl | 93.54% <ø> (ø) |
|
| src/types/SisoTfTypes/SisoZpk.jl | 63.84% <0.00%> (-17.26%) |
:arrow_down: |
| src/discrete.jl | 84.25% <0.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update e9837ee...4b40488. Read the comment docs.
So, this PR seems to have gotten outdated. Are the points adressed by it fixed, or should the changes be copied into another PR based of the current master branch?
Closed in favor of https://github.com/JuliaControl/ControlSystems.jl/pull/881