Mathew's Documentation / Code Review #369
Conversation
# Conflicts: # source/getting_started.rst # source/index.rst # source/overview.rst # source/terminology.rst
There was a problem hiding this comment.
@H0R5E I've finished my review and added inline comments delineating between comments I've addressed and ones I've suggested you submit a PR for. See full comments here: https://github.com/WEC-Sim/WEC-Sim/pull/369/files?file-filters%5B%5D=.rst
Thanks!
| MATLAB. I guess some subscribed MATLAB users might be using a 5 year old | ||
| version, but this is more likely to be users without a current subscription. | ||
| Catering to users who don't pay for the software is odd, IMO. | ||
|
|
There was a problem hiding this comment.
We've asked for feedback from users about what version of MATLAB they use, and this is driving our unit test development. Added this to the project board https://github.com/WEC-Sim/WEC-Sim/projects/49#card-47906552
| I don't think this belongs here, perhaps you need a contributing section? | ||
| Even if you have made a fork, you still need to clone it, so this isn't | ||
| really about downloading the code, as such. | ||
|
|
There was a problem hiding this comment.
I agree, we should add a section about contributing to WEC-Sim, added to the project board https://github.com/WEC-Sim/WEC-Sim/projects/50#card-48136867
| There is just as much risk that you mess something up on the trunk and break | ||
| the code of people who used option 1. A static version should provide some | ||
| security that it is relatively stable and will stay that way. | ||
|
|
There was a problem hiding this comment.
Yes, but we typically use master as our stable version, and just tag a release once we've had a good number of updates. Nonetheless this section needs to be updated, added to project board https://github.com/WEC-Sim/WEC-Sim/projects/50#card-48138603
| code actually produces as results - power production, motions, forces? It | ||
| doesn't sell why I would want to use this tool. Why would I want to use this | ||
| tool? | ||
|
|
There was a problem hiding this comment.
Please submit a PR to update the description
| script that are not explained here. | ||
|
|
||
| Also, `clc; clear all; close all;` Grrrr. | ||
|
|
There was a problem hiding this comment.
This is a bemio development comment, and could be revised. Added to project board https://github.com/WEC-Sim/WEC-Sim/projects/51#card-48138684
| .. note:: | ||
| General question - Why do the WEC-Sim library blocks not appear in the | ||
| search box when you double click the simulink workspace? | ||
|
|
There was a problem hiding this comment.
Not sure, I've never used that before.
| :style: unsrt | ||
|
|
||
| .. |tutorialspage| replace:: ``$WECSIM/tutorials`` | ||
| .. _tutorialspage: https://github.com/WEC-Sim/WEC-Sim/tree/master/tutorials |
There was a problem hiding this comment.
Is this intended to replace all local path references with pointers to the GitHub repository?
There was a problem hiding this comment.
No, I was just using it in my note. Although it is also an example of how you might have a code block act as a link.
| as a separate method would be useful. Additionally, because these classes | ||
| need configured, some pointers as to which parameters must be set, would be | ||
| helpful, IMO. | ||
|
|
There was a problem hiding this comment.
I agree, @akeeste the first part relates to regrouping public versus private methods and properties. The second part requires re-evaluating the use of the classes altogether. This could result in more functions applied universally. This is on the project board https://github.com/WEC-Sim/WEC-Sim/projects/26
| Finally, the wave class is documented differently to the other classes | ||
| because it's methods are called at the top level using automethod, rather | ||
| than included as members of the class. | ||
|
|
There was a problem hiding this comment.
Yea... this was tricky to implement with the limitations of the autodoc for MATLAB. We're open to suggested improvements, the API is a new feature as of https://github.com/WEC-Sim/WEC-Sim/releases/tag/v4.1 and could use a lot of work.
There was a problem hiding this comment.
What was your motivation for doing it differently for the Wave class?
|
Hi Kelley, sorry for some reason I missed your latest update to this. I will work on the notes you have asked me to look at. |
This commit adds the requested docs changes from PR WEC-Sim#369. Specific changes are: * Adding outputs of WEC-Sim to the introduction (index.rst) * Added info about OOP in the Overview section * Improved the mass tables in the tutorials
* Add Requested Docs Changes This commit adds the requested docs changes from PR #369. Specific changes are: * Adding outputs of WEC-Sim to the introduction (index.rst) * Added info about OOP in the Overview section * Improved the mass tables in the tutorials * Changes as per review * Wrong units for the mass * Non breaking space no longer required
|
@kmruehl, I think this is finished, would you agree? |
Hi @kmruehl, I've started my journey through the documentation of WEC-Sim, and I've decided that the easiest way to record my notes is to add them directly to the docs.
Most of the time I am using the
.. note::directive to mark it up, but I might use.. warning::as well if I need to distinguish my edits.I've only just started, but I will continue to update this PR as I work through the docs. Here is a list of the sections, to track my progress: