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

Update style guide to include documenting Attributes #20393

Open
aktech opened this issue Nov 7, 2020 · 9 comments
Open

Update style guide to include documenting Attributes #20393

aktech opened this issue Nov 7, 2020 · 9 comments

Comments

@aktech
Copy link
Member

aktech commented Nov 7, 2020

Our style guide currently doesn't have any recommendations regarding documenting class Attributes, it would be good to have that.

We can take inspiration from numpydoc: https://numpydoc.readthedocs.io/en/latest/format.html#documenting-classes

@sylee957
Copy link
Member

sylee957 commented Nov 8, 2020

I'm not sure if this is applicable we have any more attributes than .args and everything are functions because we use immutable class designs.

@oscarbenjamin
Copy link
Contributor

Some classes do have important attributes though and there isn't a clear way to document them. Often the attributes refer to the args e.g. Pow.base. The fact that there are so many attributes and methods for any Basic subclass makes them not easily discoverable so it would be good to document them in some way.

@aktech
Copy link
Member Author

aktech commented Nov 8, 2020

There are plenty of examples where this could be useful. For example:

polar = True

Also, there is nothing wrong with documenting .args either. Documentation doesn't harms, the more the better.

@sylee957
Copy link
Member

sylee957 commented Nov 8, 2020

The only problem I’d address is that we have slightly outdated builder for Attributes section than napoleon

@oscarbenjamin
Copy link
Contributor

In general I think it would be better if documentation discourages direct use of .args. A well designed Basic class should usually have properties that extract the appropriate elements from args and the properties should be used e.g.:

class Pow(Expr):
    def __new__(cls, base, exp):
        return Expr.__new__(cls, base, exp)

    @property
    def base(self):
        return self.args[0]

    @property
    def exp(self):
        return self.args[1]

    def as_base_exp(self):
        return self.base, self.exp

We should generally encourage the use of the properties base and exp or methods like as_base_exp rather than direct use of .args which should just be for low-level routines like subs or xreplace. Doing things this way makes it possible for e.g. subclasses to change the underlying args layout:

class sqrt(Pow):
    def __new__(cls, arg):
        return Expr.__new__(cls, arg)

    @property
    def base(self):
        return self.args[0]

    @property
    def exp(self):
        return S.Half

If all other code using these classes (including methods of Pow) restrict themselves to using base and exp rather than args then a subclass with different args should just work. Note that sqrt here can inherit as_base_exp with no change needed. Of course for performance reasons we might take some shortcuts and as_base_exp could return args but then need to be reimplemented in sqrt. The number of places that access args directly should be limited though.

Apart from cases like Add I think that we should generally treat the layout of .args as something of an implementation detail. The docs should encourage using attributes and methods to access the objects that are referenced in .args rather than accessing .args directly. That can only work if we document those attributes and properties though.

@moorepants
Copy link
Member

The style guide states "Everything supported by the above processing tools is available for use in the SymPy documentation, but this style guide supersedes any recommendations made in the above documents." I interpret that to mean that you can use anything from numpydoc as long as anything mentioned in the style guide is followed. So, you can add an attributes section if you need to. When we developed the guide we tried to only include things that are specifically different than the guides for sphinx, rst, numpydoc, etc.

@moorepants
Copy link
Member

Actually this statement is better "The SymPy Documentation Style Guide provides both the essential elements for writing SymPy documentation as well as any deviations in style we specify relative to these documentation processing tools." So the guide includes essentials and deviations. If you feel repeating the numpydoc attribute section in the style guide is essential, then we could do that.

@aktech
Copy link
Member Author

aktech commented Nov 10, 2020

Thanks @moorepants It makes sense. I don't feel we should be repeating it then, unless @oscarbenjamin thinks otherwise.

I guess then the only issue we have is rendering "Attributes" section properly, as noted by @sylee957 in this comment: #20331 (comment)

@moorepants
Copy link
Member

You could add a note in the style guide that links to the numpydoc attributes guide, so people will know what to look for.

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