Explixit calls for inherited methods in Rotation

[viljarjf]

Hello, just a quick question: I came over multiple inherited methods in quaternion/rotation.py, such as from_neo_euler and from_axes_angles that call the parent method immediately. Is there a specific reason for this, other than updating the docstring? All tests pass both before and after deleting the methods locally.

[hakonanes]

A good question, @viljarjf. And you're right, it's the docstring.

Yes, this adds some duplication to the code base. But, by "overwriting" the method we're able to automatically generate a complete API reference. We have complete control over which methods are listed in the API and their descriptions.

This file lists all reStructuredText code in the docs to generate the API reference: https://raw.githubusercontent.com/pyxem/orix/develop/doc/reference/index.rst. This Jinja2 file describes how a class API is documented: https://github.com/pyxem/orix/blob/develop/doc/_templates/custom-class-template.rst. We could decide to list all inherited methods, in which case we wouldn't have to "overwrite" it as we do, but we would loose some control over which methods were listed. It is also confusing to be told that a Quaternion will be returned from a Rotation method when an instance of the latter is returned.

[viljarjf]

Thanks for the reply! I think the main drawback is the duplicate type hinting, then. It seems that not adding type hinting in the overridden method makes the IDE fall back on the superclass' type hints. I can test if the docs also recognizes this

[viljarjf]

No, sadly the docs no longer contain the annotations of the parent method when given a new docstring.

There does not seem to be any way to update the docstring of inherited methods. The __doc__-attribute of methods is read-only, and instead updating the __doc__ of a method's __func__ does not work since it is the same function object as its parent (updating this also overrides the parent docstring). An alternative would be to update the annotations programatically:

    @classmethod
    def from_neo_euler(cls, neo_euler):
        """New docstring
        """
        return super().from_neo_euler(neo_euler)
    from_neo_euler.__annotations__.update(Quaternion.from_neo_euler.__annotations__)

Similar solutions are often found in other projects to inherit docstrings from wrapped functions, something like

from module import function

class A:
    def a_function():
        function()
    a_function.__doc__ = function.__doc__

but for annotations it looks uglier in my opinion. It also requires the name of the parent class, since super() cannot be used outside a function body...

It seems to me that the most pythonic way would be with decorators:

def inherit_annotations(cls):
    def wrapper(func):
        annotations = getattr(cls, func.__name__).__annotations__
        func.__annotations__ = annotations
        return func
    return wrapper

class Rotation(Quaternion):
    ...

    @classmethod
    @inherit_annotations(Quaternion)
    def from_neo_euler(cls, neo_euler):
        """New docstring
        """
        return super().from_neo_euler(neo_euler)

This still requires supplying the parent class every time. There might be some attribute trickery to get the class of a bound method, but I did not find any.

[hakonanes]

Wrapping or copying of docstrings adds unnecessary complexity in my opinion. The docstrings aren't identical either, as we use the proper noun for the class (rotations for Rotation and quaternions for Quaternion, and so on). Copying might instead lead to confusion (does a Rotation method return a Quaternion instance?).

I suggest to keep the current approach until it becomes unmanageable, which it isn't at the moment.

[viljarjf]

Sorry, I was unclear. Updating the docstrings is a good thing. There does not seem to be another way to do this, other than the current implementation. Type hints/annotations, however, can be inherited programatically. My suggestion with a decorator is only to inherit the type hints, as they are identical for the inherited methods where only the docstring is updated. The type hints would only be correct for inherited classmethods if the parent method is type hinted with a bound typing.TypeVar, or the equivalent typing.Self from python 3.11. I am in the process of replacing explicit types with those, which is where the idea came up. Programatically setting type hints allows for a little less duplicate code, while allowing the generated docs and the IDE to still recognize the type hints.

[hakonanes]

It would indeed be nice for the IDE to recognize the type hints from inherited class methods. But the important thing is for our auto generated API reference docs to be correct and complete, which it is with our current use of type hints and docstrings.

I am in the process of replacing explicit types with those, which is where the idea came up.

I haven't used TypeVar, but the documentation states that it is primarily useful for static type checkers. We don't use those (edit: apart from in IDEs, of course!), so I suggest to wait and use Self when Python 3.11 is the minimal supported version. We currently support 3.7. We usually only bump the minimal Python version when something breaks in the test suite run using that version or if we want a feature that isn't supported by the minimal version.

[viljarjf]

An example of how using a TypeVar would work in orix can be found here: https://github.com/viljarjf/orix/blob/c618b5c8b5d993345e5e9c440b0162c69e086e93/orix/quaternion/rotation.py#L175C1-L189C49

Sphinx does not interpret Self correctly, as can be seen here:

it displays Self as the output, when it should have been Rotation. My IDE interprets it correctly. As the documentation becomes less clear, and documentation is (understandably) a higher priority, I will not make a PR.

Note that, with the suggested decorator and no type hints other than in the parent method, the generated documentation looks as expected. The type hints of the parent method is indeed copied to the subclass method when using the method, and the documentation shows no type hints when none are provided and the decorator is not used. If sphinx supports the 3.11 Self eventually, then such a solution might be useful, but I'm dropping it for now.

[hakonanes]

Thank you for linking to the example. Although we don't use it now, someone might want to later on, will find this issue and your link, and will know where to start.

As you say, it might be that the Sphinx plugin responsible for the API reference (I believe it is the autodoc extension, but I'm not 100% sure) does not parse the type hints such as Self correctly. We can come back to this when Python 3.11 is the minimum supported version.

[hakonanes]

I suggest you close the issue, @viljarjf, if there is nothing more to be done. Please raise new issues if you find something you think should be changed or updated or even better, added!