ilayn / harold

An open-source systems and controls toolbox for Python3
MIT License
173 stars 19 forks source link

add docstring examples to each function #29

Open ilayn opened 6 years ago

ilayn commented 6 years ago

Current docstring contents are not sufficient. Every function, regardless of trivialness, require at least one usage example.

whzup commented 6 years ago

Hello, @ilayn I'd be down for this :smile: Although, as I'm not really well versed in Control Engineering (at the moment), I'd be glad if you could point out some easier functions I could work on for the beginning :yum:

ilayn commented 6 years ago

Hi @whzup so nice of you to offer a helping hand. You can start from anyone of them as you find interesting.

I am really hoping that being well-versed is not a prerequisite for this library. Otherwise it means that I have failed. So please let me know if you think something does not feel right or cumbersome in general. I've tried to make an introductory tutorial under notebooks folder if you wish to familiarize yourself further.

If you had the chance to tinker with the library you can add what you have done such as creating models or converting a Transfer model to a State and vice versa. Those are the equivalent functions of tf2ss and ss2tf in matlab.

I am using the rubrics of NumPy docstrings. As an example, you can add things to the last part of each docstring (before the references sections if any) as if you have typed them on the console. Take the example for least common polynomial as a template

https://github.com/ilayn/harold/blob/master/harold/_polynomial_ops.py#L76-L87

and it is rendered like this documentation page

Just a few examples sufficient at this point. We don't need to provide any tutorials (yet). Note that it is very possible that you might discover bugs or strange behavior since I've been blinded by my own design 😃 So, don't hesitate to slap me about my own stupidity.

Clone the repository and create a branch via git checkout -b <branchname> then make your changes. If you want to see locally how it would look like go to the docs folder in the repository and type make html. The resulting html pages would show up under ../_build/html/

This also tells me that I should make a contribution guide. See, you have contributed already!

When you are satisfied, you can send a PR with your additions. Let me know if you need help with the git and PR creation. It took me some time to figure it out too.

Thanks again,

whzup commented 6 years ago

@ilayn Thank you for this kind introduction!

As an example, you can add things to the last part of each docstring (before the references sections if any) as if you have typed them on the console.

So it would be possible to just copy the code from the Python Shell right?

ilayn commented 6 years ago

Yes exactly. You can also use a Jupyter notebook or Spyder but then reorganize the output as if it was coming from a shell. In other words you don't need to work on a Python shell. >>> notation is just stylistic denoting the input lines. For multilines you can use ... such as this example https://docs.scipy.org/doc/scipy/reference/generated/scipy.linalg.qr_update.html

Later on when I find the time I will include the doctesting suite such that a robot will walk through the examples sections of the each docstring and run the examples automatically comparing what it got and what it should have got. Hence we stick to "Python shell" look and feel to be future-proof.

whzup commented 6 years ago

@ilayn I just started to look around in the package and played around with the polynomial operations. I tried haroldpolyadd and I found it weird that the trim_zeros parameter doesn't trim the zeros of the output but the input. I expected it to trim the output as well. Is there a reason for that?

>>> a = np.array([2, 3, 5, 8])
>>> b = np.array([1, 3, 4])
>>> c = np.array([6, 9, 10, -8, 6])
>>> haroldpolyadd(a, b, c)
array([ 6., 11., 14.,  0., 18.])
>>> d = np.array([-2, -4 ,3, -1])
>>> haroldpolyadd(a, b, d)
array([ 0.,  0., 11., 11.])
>>> haroldpolyadd(a, b, d, trim_zeros=False)
array([ 0.,  0., 11., 11.])
>>> haroldpolyadd(a, b, d, trim_zeros=True)
array([ 0.,  0., 11., 11.])
ilayn commented 6 years ago

Ah, yes. It is only meant for output trimming. The inputs are always trimmed. Example:

>>> a = np.array([0, 0, 0])
>>> b = np.array([0, 0, 0, 1, 3, 4])
>>> c = np.array([0, 9, 10, -8, -4])
>>> haroldpolyadd(a, b, c, trim_zeros=False)
array([ 0.,  0.,  9., 11., -5., 0.])
>>> haroldpolyadd(a, b, c, trim_zeros=True)
array([ 9., 11., -5., 0.])

The reason for this optional argument is to make sure that the sizes don't change. So imagine you have two arrays of the same size, say 5, and you want to perform c = a + b. But suddenly size of c drops to 3 and you can't directly assign to a place because you thought the result would still be 5.

But I agree it is confusing. This is a perfect example why I need a fresh pair of eyes 😃 Should I write up a better docstring or would you like to have a go at it?

whzup commented 6 years ago

So looking at the source code:

def haroldpolyadd(*args, trim_zeros=True):
    """
    Similar to official polyadd from numpy but allows for
    multiple args and doesn't invert the order,
    """
    if trim_zeros:
        trimmedargs = [np.trim_zeros(x, 'f') for x in args]
    else:
        trimmedargs = args

    degs = [len(m) for m in trimmedargs]  # Get the max len of args
    s = np.zeros((1, max(degs)))
    for ind, x in enumerate(trimmedargs):
        s[0, max(degs)-degs[ind]:] += np.real(x)
    return s[0]

The if-statement should be used for the return instead of the input:

if trim_zeros:
    return np.trim_zeros(s[0], 'f')
else:
    return s[0]

I'd change this and then improve the docstring with some more information if that's alright? :smile: Maybe even a link to the numpy docs would be nice.

ilayn commented 6 years ago

The if-statement should be used for the return instead of the input:

I think I didn't understand this part but go for it anyways. We can discuss it more concretely over a PR

I'd change this and then improve the docstring with some more information if that's alright?

This is always fine 😃 Please add new tests if you add/remove functionalities under the tests folder

Maybe even a link to the numpy docs would be nice.

Very good suggestion! I will try to setup a mechanism for inter-library referrals (I think I know how to do it).