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

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

Closed
wants to merge 0 commits into from

Conversation

KronosTheLate
Copy link
Contributor

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
Copy link
Contributor Author

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.

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

coveralls commented Oct 5, 2020

Coverage Status

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

Copy link
Contributor

@olof3 olof3 left a comment

Choose a reason for hiding this comment

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

Thank you for giving some love and care to the docs! (they certainly need it)

docs/src/man/creating_systems.md Outdated Show resolved Hide resolved
docs/src/man/creating_systems.md Outdated Show resolved Hide resolved
docs/src/man/creating_systems.md Outdated Show resolved Hide resolved
docs/src/man/creating_systems.md Outdated Show resolved Hide resolved
src/timeresp.jl Outdated Show resolved Hide resolved
src/timeresp.jl Outdated Show resolved Hide resolved
Copy link
Member

@baggepinnen baggepinnen left a comment

Choose a reason for hiding this comment

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

Looks good. I just want to double check with @mfalt so that the #output syntax is not needed for Documenter.jl doctests?

docs/src/man/creating_systems.md Outdated Show resolved Hide resolved
docs/src/man/creating_systems.md Outdated Show resolved Hide resolved
@KronosTheLate
Copy link
Contributor Author

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

@mfalt
Copy link
Member

mfalt commented Oct 5, 2020

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 #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
Copy link
Member

mfalt commented Oct 9, 2020

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link

codecov bot commented Oct 29, 2020

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%) ⬇️
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.

@KronosTheLate
Copy link
Contributor Author

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
Copy link
Contributor Author

Closed in favor of #881

baggepinnen pushed a commit that referenced this pull request Oct 11, 2023
* incorporate changes from 344

* Restore first heading

* Restore second heading
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

5 participants