Repository: pydata/xarray

Issue:
Documentation for injected methods
At the moment, the documentation for methods introduced by accessors is broken. We work around this issue by directly referencing the corresponding accessor method or the underlying function. Of course, this is not how we recommend calling them, so we should try to fix this.

It seems this is mostly a sphinx issue since this is known (see #3333 and #3602), but we didn't find a way to resolve this. I'd like use this issue to collect ideas for solutions.

Discussion:
keewis: there are a few of my questions / comments we can resolve by using something like
```python
class UncachedAccessor:
    def __init__(self, accessor):
        self._accessor = accessor

    def __get__(self, obj, cls):
        if obj is None:
            return self._accessor

        return self._accessor(obj)
```

The only disadvantage is that now `pydoc` (or `help()`) show the accessor as a class. That was already the case for `str` and `dt`, but `plot` was a function.

I think we could try to trick `inspect.getmembers` (which is used to decide which kind of output should be done), but that would require a lot of monkeypatching I'd rather not do.

`pandas` simply uses the same docstring for the class and `__call__`, which is not ideal but a lot simpler.
shoyer: I think pandas might do something similar to do this? It might be worth checking, though it looks like you've figured most of it out already :)
shoyer: This is a really clever yet simple fix. I like it!

A few ideas for testing:

- Verify that `DataArray.plot` returns the PlotMethods object.
- Verify that `data_array.plot.__doc__` is still there.
shoyer: > there are a few of my questions / comments we can resolve by using something like
> 
> ```python
> class UncachedAccessor:
>     def __init__(self, accessor):
>         self._accessor = accessor
> 
>     def __get__(self, obj, cls):
>         if obj is None:
>             return self._accessor
> 
>         return self._accessor(obj)
> ```
> 
> The only disadvantage is that now `pydoc` (or `help()`) show the accessor as a class. That was already the case for `str` and `dt`, but `plot` was a function.
> 
> I think we could try to trick `inspect.getmembers` (which is used to decide which kind of output should be done), but that would require a lot of monkeypatching I'd rather not do.
> 
> `pandas` simply uses the same docstring for the class and `__call__`, which is not ideal but a lot simpler.

👍  for a simple solution here like pandas rather than using inspect. I think it's probably actually slightly more accurate to show the class.
keewis: where should the docstring of the accessor come from? Right now, this is the docstring of the return value which seems reasonable to me.

I added tests verifying that `cls.property` returns the accessor class and `obj.property` the accessor instance. Do we need additional checks for `__doc__`?.
keewis: I added the accessor templates from `pandas` (we should eventually outsource these to a [separate package](https://github.com/pydata/xarray/issues/3361#issuecomment-618338469) used by both `pandas` and `xarray`) and modified the [API page](http://xarray.pydata.org/en/accessor-documentation/api.html#plotting) to display the accessor version of the plotting functions.

I think we should add a accessor section to each of the main objects (`Dataset` and `DataArray`), where we either copy what `pandas` does – listing every method / attribute of the accessor as a subsection of the class documentation – or we could show them as attributes (just like what we do for the injected numpy functions) and generate separate pages for the accessors.

Patch:
diff --git a/doc/_templates/autosummary/accessor.rst b/doc/_templates/autosummary/accessor.rst
new file mode 100644
index 00000000..4ba745cd
--- /dev/null
+++ b/doc/_templates/autosummary/accessor.rst
@@ -0,0 +1,6 @@
+{{ fullname }}
+{{ underline }}
+
+.. currentmodule:: {{ module.split('.')[0] }}
+
+.. autoaccessor:: {{ (module.split('.')[1:] + [objname]) | join('.') }}
diff --git a/doc/_templates/autosummary/accessor_attribute.rst b/doc/_templates/autosummary/accessor_attribute.rst
new file mode 100644
index 00000000..b5ad65d6
--- /dev/null
+++ b/doc/_templates/autosummary/accessor_attribute.rst
@@ -0,0 +1,6 @@
+{{ fullname }}
+{{ underline }}
+
+.. currentmodule:: {{ module.split('.')[0] }}
+
+.. autoaccessorattribute:: {{ (module.split('.')[1:] + [objname]) | join('.') }}
diff --git a/doc/_templates/autosummary/accessor_callable.rst b/doc/_templates/autosummary/accessor_callable.rst
new file mode 100644
index 00000000..7a330181
--- /dev/null
+++ b/doc/_templates/autosummary/accessor_callable.rst
@@ -0,0 +1,6 @@
+{{ fullname }}
+{{ underline }}
+
+.. currentmodule:: {{ module.split('.')[0] }}
+
+.. autoaccessorcallable:: {{ (module.split('.')[1:] + [objname]) | join('.') }}.__call__
diff --git a/doc/_templates/autosummary/accessor_method.rst b/doc/_templates/autosummary/accessor_method.rst
new file mode 100644
index 00000000..aefbba6e
--- /dev/null
+++ b/doc/_templates/autosummary/accessor_method.rst
@@ -0,0 +1,6 @@
+{{ fullname }}
+{{ underline }}
+
+.. currentmodule:: {{ module.split('.')[0] }}
+
+.. autoaccessormethod:: {{ (module.split('.')[1:] + [objname]) | join('.') }}
diff --git a/doc/api.rst b/doc/api.rst
index 3f25ac1a..bb0edd0d 100644
--- a/doc/api.rst
+++ b/doc/api.rst
@@ -233,6 +233,15 @@ Reshaping and reorganizing
    Dataset.sortby
    Dataset.broadcast_like
 
+Plotting
+--------
+
+.. autosummary::
+   :toctree: generated/
+   :template: autosummary/accessor_method.rst
+
+   Dataset.plot.scatter
+
 DataArray
 =========
 
@@ -403,6 +412,122 @@ Computation
 :py:attr:`~core.groupby.DataArrayGroupBy.where`
 :py:attr:`~core.groupby.DataArrayGroupBy.quantile`
 
+
+String manipulation
+-------------------
+
+.. autosummary::
+   :toctree: generated/
+   :template: autosummary/accessor_method.rst
+
+   DataArray.str.capitalize
+   DataArray.str.center
+   DataArray.str.contains
+   DataArray.str.count
+   DataArray.str.decode
+   DataArray.str.encode
+   DataArray.str.endswith
+   DataArray.str.find
+   DataArray.str.get
+   DataArray.str.index
+   DataArray.str.isalnum
+   DataArray.str.isalpha
+   DataArray.str.isdecimal
+   DataArray.str.isdigit
+   DataArray.str.isnumeric
+   DataArray.str.isspace
+   DataArray.str.istitle
+   DataArray.str.isupper
+   DataArray.str.len
+   DataArray.str.ljust
+   DataArray.str.lower
+   DataArray.str.lstrip
+   DataArray.str.match
+   DataArray.str.pad
+   DataArray.str.repeat
+   DataArray.str.replace
+   DataArray.str.rfind
+   DataArray.str.rindex
+   DataArray.str.rjust
+   DataArray.str.rstrip
+   DataArray.str.slice
+   DataArray.str.slice_replace
+   DataArray.str.startswith
+   DataArray.str.strip
+   DataArray.str.swapcase
+   DataArray.str.title
+   DataArray.str.translate
+   DataArray.str.upper
+   DataArray.str.wrap
+   DataArray.str.zfill
+
+Datetimelike properties
+-----------------------
+
+**Datetime properties**:
+
+.. autosummary::
+   :toctree: generated/
+   :template: autosummary/accessor_attribute.rst
+
+   DataArray.dt.year
+   DataArray.dt.month
+   DataArray.dt.day
+   DataArray.dt.hour
+   DataArray.dt.minute
+   DataArray.dt.second
+   DataArray.dt.microsecond
+   DataArray.dt.nanosecond
+   DataArray.dt.weekofyear
+   DataArray.dt.week
+   DataArray.dt.dayofweek
+   DataArray.dt.weekday
+   DataArray.dt.weekday_name
+   DataArray.dt.dayofyear
+   DataArray.dt.quarter
+   DataArray.dt.days_in_month
+   DataArray.dt.daysinmonth
+   DataArray.dt.season
+   DataArray.dt.time
+   DataArray.dt.is_month_start
+   DataArray.dt.is_month_end
+   DataArray.dt.is_quarter_end
+   DataArray.dt.is_year_start
+   DataArray.dt.is_leap_year
+
+**Datetime methods**:
+
+.. autosummary::
+   :toctree: generated/
+   :template: autosummary/accessor_method.rst
+
+   DataArray.dt.floor
+   DataArray.dt.ceil
+   DataArray.dt.round
+   DataArray.dt.strftime
+
+**Timedelta properties**:
+
+.. autosummary::
+   :toctree: generated/
+   :template: autosummary/accessor_attribute.rst
+
+   DataArray.dt.days
+   DataArray.dt.seconds
+   DataArray.dt.microseconds
+   DataArray.dt.nanoseconds
+
+**Timedelta methods**:
+
+.. autosummary::
+   :toctree: generated/
+   :template: autosummary/accessor_method.rst
+
+   DataArray.dt.floor
+   DataArray.dt.ceil
+   DataArray.dt.round
+
+
 Reshaping and reorganizing
 --------------------------
 
@@ -419,6 +544,27 @@ Reshaping and reorganizing
    DataArray.sortby
    DataArray.broadcast_like
 
+Plotting
+--------
+
+.. autosummary::
+   :toctree: generated/
+   :template: autosummary/accessor_callable.rst
+
+   DataArray.plot
+
+.. autosummary::
+   :toctree: generated/
+   :template: autosummary/accessor_method.rst
+
+   DataArray.plot.contourf
+   DataArray.plot.contour
+   DataArray.plot.hist
+   DataArray.plot.imshow
+   DataArray.plot.line
+   DataArray.plot.pcolormesh
+   DataArray.plot.step
+
 .. _api.ufuncs:
 
 Universal functions
@@ -664,25 +810,6 @@ Creating custom indexes
 
    cftime_range
 
-Plotting
-========
-
-.. autosummary::
-   :toctree: generated/
-
-   Dataset.plot
-   plot.scatter
-   DataArray.plot
-   plot.plot
-   plot.contourf
-   plot.contour
-   plot.hist
-   plot.imshow
-   plot.line
-   plot.pcolormesh
-   plot.step
-   plot.FacetGrid
-
 Faceting
 --------
 .. autosummary::
diff --git a/doc/conf.py b/doc/conf.py
index 6b16468d..d3d126cb 100644
--- a/doc/conf.py
+++ b/doc/conf.py
@@ -20,6 +20,12 @@ import subprocess
 import sys
 from contextlib import suppress
 
+# --------- autosummary templates ------------------
+# TODO: eventually replace this with a sphinx.ext.auto_accessor module
+import sphinx
+from sphinx.ext.autodoc import AttributeDocumenter, Documenter, MethodDocumenter
+from sphinx.util import rpartition
+
 # make sure the source version is preferred (#3567)
 root = pathlib.Path(__file__).absolute().parent.parent
 os.environ["PYTHONPATH"] = str(root)
@@ -358,3 +364,113 @@ intersphinx_mapping = {
     "dask": ("https://docs.dask.org/en/latest", None),
     "cftime": ("https://unidata.github.io/cftime", None),
 }
+
+
+# --------- autosummary templates ------------------
+# TODO: eventually replace this with a sphinx.ext.auto_accessor module
+class AccessorDocumenter(MethodDocumenter):
+    """
+    Specialized Documenter subclass for accessors.
+    """
+
+    objtype = "accessor"
+    directivetype = "method"
+
+    # lower than MethodDocumenter so this is not chosen for normal methods
+    priority = 0.6
+
+    def format_signature(self):
+        # this method gives an error/warning for the accessors, therefore
+        # overriding it (accessor has no arguments)
+        return ""
+
+
+class AccessorLevelDocumenter(Documenter):
+    """
+    Specialized Documenter subclass for objects on accessor level (methods,
+    attributes).
+    """
+
+    # This is the simple straightforward version
+    # modname is None, base the last elements (eg 'hour')
+    # and path the part before (eg 'Series.dt')
+    # def resolve_name(self, modname, parents, path, base):
+    #     modname = 'pandas'
+    #     mod_cls = path.rstrip('.')
+    #     mod_cls = mod_cls.split('.')
+    #
+    #     return modname, mod_cls + [base]
+
+    def resolve_name(self, modname, parents, path, base):
+        if modname is None:
+            if path:
+                mod_cls = path.rstrip(".")
+            else:
+                mod_cls = None
+                # if documenting a class-level object without path,
+                # there must be a current class, either from a parent
+                # auto directive ...
+                mod_cls = self.env.temp_data.get("autodoc:class")
+                # ... or from a class directive
+                if mod_cls is None:
+                    mod_cls = self.env.temp_data.get("py:class")
+                # ... if still None, there's no way to know
+                if mod_cls is None:
+                    return None, []
+            # HACK: this is added in comparison to ClassLevelDocumenter
+            # mod_cls still exists of class.accessor, so an extra
+            # rpartition is needed
+            modname, accessor = rpartition(mod_cls, ".")
+            modname, cls = rpartition(modname, ".")
+            parents = [cls, accessor]
+            # if the module name is still missing, get it like above
+            if not modname:
+                modname = self.env.temp_data.get("autodoc:module")
+            if not modname:
+                if sphinx.__version__ > "1.3":
+                    modname = self.env.ref_context.get("py:module")
+                else:
+                    modname = self.env.temp_data.get("py:module")
+            # ... else, it stays None, which means invalid
+        return modname, parents + [base]
+
+
+class AccessorAttributeDocumenter(AccessorLevelDocumenter, AttributeDocumenter):
+
+    objtype = "accessorattribute"
+    directivetype = "attribute"
+
+    # lower than AttributeDocumenter so this is not chosen for normal attributes
+    priority = 0.6
+
+
+class AccessorMethodDocumenter(AccessorLevelDocumenter, MethodDocumenter):
+
+    objtype = "accessormethod"
+    directivetype = "method"
+
+    # lower than MethodDocumenter so this is not chosen for normal methods
+    priority = 0.6
+
+
+class AccessorCallableDocumenter(AccessorLevelDocumenter, MethodDocumenter):
+    """
+    This documenter lets us removes .__call__ from the method signature for
+    callable accessors like Series.plot
+    """
+
+    objtype = "accessorcallable"
+    directivetype = "method"
+
+    # lower than MethodDocumenter; otherwise the doc build prints warnings
+    priority = 0.5
+
+    def format_name(self):
+        return MethodDocumenter.format_name(self).rstrip(".__call__")
+
+
+def setup(app):
+    app.add_autodocumenter(AccessorDocumenter)
+    app.add_autodocumenter(AccessorAttributeDocumenter)
+    app.add_autodocumenter(AccessorMethodDocumenter)
+    app.add_autodocumenter(AccessorCallableDocumenter)
diff --git a/doc/whats-new.rst b/doc/whats-new.rst
index 68b2d738..dade282d 100644
--- a/doc/whats-new.rst
+++ b/doc/whats-new.rst
@@ -174,6 +174,8 @@ Documentation
   By `Justus Magin <https://github.com/keewis>`_.
 - Narrative documentation now describes :py:meth:`map_blocks`: :ref:`dask.automatic-parallelization`.
   By `Deepak Cherian <https://github.com/dcherian>`_.
+- Document ``.plot``, ``.dt``, ``.str`` accessors the way they are called. (:issue:`3625`, :pull:`3988`)
+  By `Justus Magin <https://github.com/keewis>`_.
 - Add documentation for the parameters and return values of :py:meth:`DataArray.sel`.
   By `Justus Magin <https://github.com/keewis>`_.
 
diff --git a/xarray/core/dataarray.py b/xarray/core/dataarray.py
index 44773e36..5814c828 100644
--- a/xarray/core/dataarray.py
+++ b/xarray/core/dataarray.py
@@ -260,7 +260,7 @@ class DataArray(AbstractArray, DataWithCoords):
     _resample_cls = resample.DataArrayResample
     _weighted_cls = weighted.DataArrayWeighted
 
-    dt = property(CombinedDatetimelikeAccessor)
+    dt = utils.UncachedAccessor(CombinedDatetimelikeAccessor)
 
     def __init__(
         self,
@@ -2722,24 +2722,7 @@ class DataArray(AbstractArray, DataWithCoords):
     def _copy_attrs_from(self, other: Union["DataArray", Dataset, Variable]) -> None:
         self.attrs = other.attrs
 
-    @property
-    def plot(self) -> _PlotMethods:
-        """
-        Access plotting functions for DataArray's
-
-        >>> d = xr.DataArray([[1, 2], [3, 4]])
-
-        For convenience just call this directly
-
-        >>> d.plot()
-
-        Or use it as a namespace to use xarray.plot functions as
-        DataArray methods
-
-        >>> d.plot.imshow()  # equivalent to xarray.plot.imshow(d)
-
-        """
-        return _PlotMethods(self)
+    plot = utils.UncachedAccessor(_PlotMethods)
 
     def _title_for_slice(self, truncate: int = 50) -> str:
         """
@@ -3831,7 +3814,7 @@ class DataArray(AbstractArray, DataWithCoords):
 
     # this needs to be at the end, or mypy will confuse with `str`
     # https://mypy.readthedocs.io/en/latest/common_issues.html#dealing-with-conflicting-names
-    str = property(StringAccessor)
+    str = utils.UncachedAccessor(StringAccessor)
 
 
 # priority most be higher than Variable to properly work with binary ufuncs
diff --git a/xarray/core/dataset.py b/xarray/core/dataset.py
index a8011afd..2958cad8 100644
--- a/xarray/core/dataset.py
+++ b/xarray/core/dataset.py
@@ -5563,16 +5563,7 @@ class Dataset(Mapping, ImplementsDatasetReduce, DataWithCoords):
     def imag(self):
         return self._unary_op(lambda x: x.imag, keep_attrs=True)(self)
 
-    @property
-    def plot(self):
-        """
-        Access plotting functions for Datasets.
-        Use it as a namespace to use xarray.plot functions as Dataset methods
-
-        >>> ds.plot.scatter(...)  # equivalent to xarray.plot.scatter(ds,...)
-
-        """
-        return _Dataset_PlotMethods(self)
+    plot = utils.UncachedAccessor(_Dataset_PlotMethods)
 
     def filter_by_attrs(self, **kwargs):
         """Returns a ``Dataset`` with variables that match specific conditions.
diff --git a/xarray/core/utils.py b/xarray/core/utils.py
index 1126cf30..0542f850 100644
--- a/xarray/core/utils.py
+++ b/xarray/core/utils.py
@@ -787,6 +787,24 @@ def drop_dims_from_indexers(
         )
 
 
+class UncachedAccessor:
+    """ Acts like a property, but on both classes and class instances
+
+    This class is necessary because some tools (e.g. pydoc and sphinx)
+    inspect classes for which property returns itself and not the
+    accessor.
+    """
+
+    def __init__(self, accessor):
+        self._accessor = accessor
+
+    def __get__(self, obj, cls):
+        if obj is None:
+            return self._accessor
+
+        return self._accessor(obj)
+
+
 # Singleton type, as per https://github.com/python/typing/pull/240
 class Default(Enum):
     token = 0
diff --git a/xarray/plot/plot.py b/xarray/plot/plot.py
index 19a3f1e6..e4a981da 100644
--- a/xarray/plot/plot.py
+++ b/xarray/plot/plot.py
@@ -445,6 +445,11 @@ class _PlotMethods:
     def __call__(self, **kwargs):
         return plot(self._da, **kwargs)
 
+    # we can't use functools.wraps here since that also modifies the name / qualname
+    __doc__ = __call__.__doc__ = plot.__doc__
+    __call__.__wrapped__ = plot  # type: ignore
+    __call__.__annotations__ = plot.__annotations__
+
     @functools.wraps(hist)
     def hist(self, ax=None, **kwargs):
         return hist(self._da, ax=ax, **kwargs)


Test patch:
diff --git a/xarray/tests/test_plot.py b/xarray/tests/test_plot.py
index 6497987e..c26d105a 100644
--- a/xarray/tests/test_plot.py
+++ b/xarray/tests/test_plot.py
@@ -111,6 +111,12 @@ class TestPlot(PlotTestCase):
     def setup_array(self):
         self.darray = DataArray(easy_array((2, 3, 4)))
 
+    def test_accessor(self):
+        from ..plot.plot import _PlotMethods
+
+        assert DataArray.plot is _PlotMethods
+        assert isinstance(self.darray.plot, _PlotMethods)
+
     def test_label_from_attrs(self):
         da = self.darray.copy()
         assert "" == label_from_attrs(da)
@@ -2098,6 +2104,12 @@ class TestDatasetScatterPlots(PlotTestCase):
         ds.B.attrs["units"] = "Bunits"
         self.ds = ds
 
+    def test_accessor(self):
+        from ..plot.dataset_plot import _Dataset_PlotMethods
+
+        assert Dataset.plot is _Dataset_PlotMethods
+        assert isinstance(self.ds.plot, _Dataset_PlotMethods)
+
     @pytest.mark.parametrize(
         "add_guide, hue_style, legend, colorbar",
         [