ControlSystems.jl icon indicating copy to clipboard operation
ControlSystems.jl copied to clipboard

Improve documentation for tf(), step, stepplot, impulse and impulseplot

Open KronosTheLate opened this issue 5 years ago • 10 comments

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"

KronosTheLate avatar Oct 05 '20 15:10 KronosTheLate

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.

KronosTheLate avatar Oct 05 '20 15:10 KronosTheLate

Coverage Status

Coverage decreased (-2.1%) to 55.538% when pulling 0b6ca0c6c23eb437a7894c64b4f1e9953697bfd6 on KronosTheLate:master into 0f3c31376866d7f68070a43eadf59cd7f8623f76 on JuliaControl:master.

coveralls avatar Oct 05 '20 15:10 coveralls

Coverage Status

Coverage remained the same at 57.791% when pulling ad9a4abcca1a8092b6d5ee7c3c77d1ea8b196114 on KronosTheLate:master into e993f7be4d287d1b517b02933f985d99ea5cf4b7 on JuliaControl:master.

coveralls avatar Oct 05 '20 15:10 coveralls

@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 😃

KronosTheLate avatar Oct 05 '20 18:10 KronosTheLate

Looks good. I just want to double check with @mfalt so that the #output syntax 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

mfalt avatar Oct 05 '20 22:10 mfalt

@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.

mfalt avatar Oct 09 '20 11:10 mfalt

@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 ^_^

KronosTheLate avatar Oct 11 '20 11:10 KronosTheLate

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.

KronosTheLate avatar Oct 29 '20 16:10 KronosTheLate

Codecov Report

Merging #344 (78d68cb) into master (e9837ee) will decrease coverage by 0.79%. The diff coverage is n/a.

Impacted file tree graph

@@            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 data Powered by Codecov. Last update e9837ee...4b40488. Read the comment docs.

codecov[bot] avatar Oct 29 '20 16:10 codecov[bot]

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?

KronosTheLate avatar Jun 03 '21 12:06 KronosTheLate

Closed in favor of https://github.com/JuliaControl/ControlSystems.jl/pull/881

KronosTheLate avatar Oct 10 '23 13:10 KronosTheLate