Skip to content

Conversation

@stephenworsley
Copy link
Contributor

This addresses #3410 by making __init__ in Coord an abstractmethod.

@stephenworsley stephenworsley added this to the v3.0.0 milestone Nov 8, 2019
@stephenworsley
Copy link
Contributor Author

One thing to bear in mind about this approach is that the __doc__ of AuxCoord.__init__ will be overwritten. Though inspect.getdoc(AuxCoord.__init__) still inherits the docstring properly, so this may be enough for things like Sphinx.

@stephenworsley
Copy link
Contributor Author

It might also make sense to make _DimensionalMetadata properly abstract. It currently derives from ABCMeta but has no abstract methods, so it can still be instantiated.

@bjlittle
Copy link
Member

@stephenworsley I played with a couple of options for the AuxCoord doc-string.

from functools import wraps
...
class AuxCoord(Coord):
    @wraps(Coord.__init__)
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)

Works fine, but has a minor performance overhead:

timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[10]: 11.958185139999841
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[11]: 11.816572594994796
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[12]: 11.72259231901262
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[13]: 11.927083795992075

This is compared with master:

timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[5]: 11.874402218993055
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[6]: 11.801809267999488
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[7]: 11.369123923010193
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[8]: 11.455261625000276

An alternative is:

class AuxCoord(Coord):
    def __init__(self, *args, **kwargs):
        self.__doc__ = super().__init__.__doc__
        super().__init__(*args, **kwargs)

which has the following behaviour:

timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[7]: 12.073399219996645
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[8]: 12.17960434599081
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[9]: 12.184167061001062
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[10]: 12.351267233985709

Or indeed, there is this:

class AuxCoord(Coord):
    def __init__(self, *args, **kwargs):
        self.__doc__ = Coord.__init__.__doc__
        super().__init__(*args, **kwargs)

which has the following behaviour:

timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[10]: 11.896369188994868
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[11]: 12.091242638998665
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[12]: 12.14877037498809
timeit('AuxCoord(1)', setup='from iris.coords import AuxCoord')
Out[13]: 12.003538096003467

Note that timeit is executing the AuxCoord(1) statement 1,000,000 times by default.

Given the above, I'd be happy to opt for the first option using @wraps

@bjlittle
Copy link
Member

bjlittle commented Nov 11, 2019

@stephenworsley Could you please make an issue to cover _DimensionalMetadata, and reference this PR in it. Thanks 👍

@stephenworsley
Copy link
Contributor Author

It looks like using @wraps causes @abstractmethod to carry over as well.

@stephenworsley stephenworsley force-pushed the abstract_coord branch 2 times, most recently from 4ec600b to dbce081 Compare November 12, 2019 12:17
"""

@wraps(Coord.__init__, assigned=("__doc__",), updated=())
Copy link
Member

@bjlittle bjlittle Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjlittle bjlittle merged commit a800c33 into SciTools:master Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants