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

Convert Docstrings to Numpy Style #347

Open
meatballs opened this issue Oct 6, 2015 · 25 comments
Open

Convert Docstrings to Numpy Style #347

meatballs opened this issue Oct 6, 2015 · 25 comments

Comments

@meatballs
Copy link
Member

Our docstrings currently use a variety of formats and we should agree a standard.

My suggestion is that we use Numpydoc (/~https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt) since it's the closest to anything we currently have.

Tasks required are:

  1. Agree the standard we're adopting
  2. Tidy up the existing docstrings to meet that standard
  3. Add some instructions to docs/contributing.rst
@drvinceknight
Copy link
Member

I'm happy with the suggested format.

@langner
Copy link
Member

langner commented Oct 6, 2015

+1

1 similar comment
@marcharper
Copy link
Member

👍

@marcharper marcharper changed the title Docstring Format Convert Docstrings to Numpy Style Oct 14, 2015
@meatballs
Copy link
Member Author

I reckon we can close this one now. Anyone object?

@drvinceknight
Copy link
Member

Fine by me.

@marcharper
Copy link
Member

Have we converted them all?

@drvinceknight
Copy link
Member

Fair point. I don't believe we have for everything. Main components: I
believe we have but not all strategies for example.

On Mon, 25 Jul 2016, 16:56 Marc Harper, notifications@github.com wrote:

Have we converted them all?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#347 (comment),
or mute the thread
/~https://github.com/notifications/unsubscribe-auth/ACCGWiYt3hoILfeden-S_5JWSBwRWPFvks5qZNy3gaJpZM4GJk94
.

@drvinceknight
Copy link
Member

I'll pick this up next.

@drvinceknight drvinceknight self-assigned this Nov 3, 2016
@meatballs
Copy link
Member Author

What's your opinion on my last comment on #457?

I suspect using annotations might be a better option.

@drvinceknight
Copy link
Member

What's your opinion on my last comment on #457?

So the idea would be that the docstrings would no longer need:

Parameters
-----------

or

Returns
--------

is that the idea?

I guess when viewing the docstrings (via a notebook for example) the type annotation would be visible so that that would not be 'lost' and we would still have a docstring that describes what is going on right?

Could you explain what we would use that autodoc plugin for? We're not currently autodoc'ing anything apart from the strategies.

@meatballs
Copy link
Member Author

there's quite a good example here: https://pypi.python.org/pypi/sphinx-autodoc-napoleon-typehints

@meatballs
Copy link
Member Author

The docstrings remain but argument types and return value types move into the annotations.

@drvinceknight
Copy link
Member

The docstrings remain but argument types and return value types move into the annotations.

Just so that I understand, the advantage of that is two fold:

  1. We don't have to have Parameters and Returns which is what the numpy spec is about.
  2. By getting rid of those we ensure that they are never lies (which as and when we've changed things sometimes they have become).

there's quite a good example here: https://pypi.python.org/pypi/sphinx-autodoc-napoleon-typehints

That's a different plugin to the one you pasted on #457 right?

I'm still not sure I understand what the purpose of it is (having looked through both of the plugins). Is that actually rewriting the docstring (:confused:) in the source code or is it for the purpose of autodoc'ing when it comes to our documentation? (Just attempting to understand.)

@meatballs
Copy link
Member Author

Yes, there are a couple of different sphinx plugins doing the same job. That last one just had a nicer description.

@meatballs
Copy link
Member Author

We don't have to have Parameters and Returns which is what the numpy spec is about.

No, we still keep the Parameters and Returns, but they no longer specify the types of each entry. That's done instead within the annotations

@meatballs
Copy link
Member Author

meatballs commented Nov 3, 2016

An example from match.py:

The current docstring for the init method starts with

Parameters
----------
        players : tuple
            A pair of axelrod.Player objects
        turns : integer
            The number of turns per match

which defines the type of each parameter. This would become:

Parameters
----------
        players : A pair of axelrod.Player objects
        turns : The number of turns per match

@drvinceknight
Copy link
Member

Thanks for clarifying, I understand now.

I'll simply go through and add the Parameters and Returns where they are absent.

@theref
Copy link
Member

theref commented Nov 4, 2016

Is this how new contributions should be documented?

@drvinceknight
Copy link
Member

I would suggest we keep with the status quo for now (so use numpy style). Until we actually change things it's best to stick with the same message.

@meatballs
Copy link
Member Author

Ordinary Numpy docstrings please. The new style will only become possible once we drop python 2.

@langner
Copy link
Member

langner commented Aug 15, 2018

Latest numpydoc docs are at https://numpydoc.readthedocs.io/en/latest/

@drvinceknight
Copy link
Member

If/when someone works on this could I suggest we don't include variable types for cases where type annotations have been included?

@langner
Copy link
Member

langner commented Aug 15, 2018

I thought about doing this while rereading he library. Started with actions in #1193. Vince, while doing that I did think that types are redundant in cases where annotation are already there. In that case what would the format for returns be? Let's look at actions_to_str as a concrete example, if needed.

@drvinceknight
Copy link
Member

drvinceknight commented Aug 15, 2018

Awesome! Would be really cool if you could pick this up :)

So currently we have:

def str_to_actions(actions: str) -> tuple:
    """Takes a string like 'CCDD' and returns a tuple of the appropriate
    actions."""
return tuple(Action.from_char(element) for element in actions)

My suggestion would be to match numpy style when the type is omitted: see 4. Parameters of https://numpydoc.readthedocs.io/en/latest/format.html

which has:

Parameters
----------
x : type
    Description of parameter `x`.
y
    Description of parameter `y` (with type not specified)

So in our case I think this would be something like:

def str_to_actions(actions: str) -> tuple:
    """
    Takes a string like 'CCDD' and returns a tuple of the appropriate
    actions.

    Parameters
    ---------------
    
    actions 
                  a string of actions

    Returns: 
    ----------
    
                  a tuple containing actions
    """
return tuple(Action.from_char(element) for element in actions)

@marcharper
Copy link
Member

marcharper commented Aug 16, 2018

Also feel free to improve the type hints. For example in this case it's a tuple of Actions, which can be specified more precisely than tuple IIUC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants