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

The use of @cache_readonly removes the docstring of attributes #18197

Closed
jorisvandenbossche opened this issue Nov 9, 2017 · 6 comments
Closed
Labels
Milestone

Comments

@jorisvandenbossche
Copy link
Member

Related to our general investigations into the api docs (#18147), there is the problem that those attributes that are decorated with @cache_readonly no longer have a docstring. Eg:

In [1]: pd.Index.is_unique?
Type:        NoneType
String form: None
Docstring:   <no docstring>

If I simply comment out the cache_readonly for this attribute (just as a check this is the culprit), I get:

In [1]: pd.Index.is_unique?
Signature: pd.Index.is_unique(self)
Docstring: return if the index has unique values 
File:      ~/scipy/pandas/pandas/core/indexes/base.py
Type:      function

(although it seems to think it is a function, although inspect.isfunction correctly gives false)

Just posting this here as a reminder to further investigate, and to see if somebody directly has an idea how to fix this (@jreback ?). The main problem is that this decorator is implemented in cython, and we probably can't use something like functools.wraps.

@jreback
Copy link
Contributor

jreback commented Nov 9, 2017

I think we could do what wraps is doing though
which is just setting dunder doc I think

@jorisvandenbossche
Copy link
Member Author

which is just setting dunder doc I think

I tried to do that back then, but that gave compile errors (to be correct: no error on compilation, but on import I got an error that looked like when you didn't compile correctly). So not sure what was going on there.

@jorisvandenbossche
Copy link
Member Author

OK, I forgot to declare the new doc property I added :-)

Trying it now correctly, I managed to do something like the following:

diff --git a/pandas/_libs/properties.pyx b/pandas/_libs/properties.pyx
index 4beb24f..37886c6 100644
--- a/pandas/_libs/properties.pyx
+++ b/pandas/_libs/properties.pyx
@@ -9,17 +9,19 @@ from cpython cimport (
 cdef class cache_readonly(object):
 
     cdef readonly:
-        object func, name, allow_setting
+        object func, name, doc, allow_setting
 
     def __init__(self, func=None, allow_setting=False):
         if func is not None:
             self.func = func
             self.name = func.__name__
+            self.doc = getattr(func, '__doc__', None)
         self.allow_setting = allow_setting
 
     def __call__(self, func, doc=None):
         self.func = func
         self.name = func.__name__
+        self.doc = getattr(func, '__doc__', None)
         return self
 
     def __get__(self, obj, typ):
@@ -30,7 +32,7 @@ cdef class cache_readonly(object):
             try:
                 cache = obj._cache = {}
             except (AttributeError):
-                return
+                return type('cached', (object, ), {'__doc__': self.doc})
 
         if PyDict_Contains(cache, self.name):
             # not necessary to Py_INCREF

Which gives:

In [1]: pd.Index.is_unique?
Init signature: pd.Index.is_unique()
Docstring:      return if the index has unique values 
File:           ~/miniconda3/envs/dev/lib/python3.5/site-packages/IPython/core/interactiveshell.py
Type:           type

which has the correct docstring, but the rest is also not ideal.

As comparison, the original current result is:

In [1]: pd.Index.is_unique?
Type:        NoneType
String form: None
Docstring:   <no docstring>

So the main problem is that None is returned from the cache_readonly decorated property if there is no actual object with cache yet (so eg on the class). And, you cannot add a docstring to None of course (I tried adding an doctring to a dummy = object() instance to return, but that also raises an error)

So with the type('cached', (object, ), {'__doc__': self.doc}) from the diff, I try to quickly create a dummy class with the desired docstring to return.
Somebody an idea to return such a class to hold the docstring in a better way ?

@jorisvandenbossche
Copy link
Member Author

@jreback @TomAugspurger any ideas on this one?
It would be nice to fix this before the doc sprint

@jbrockmendel
Copy link
Member

#19709 had a fix for this.

@jorisvandenbossche
Copy link
Member Author

Ah, I am doing basically the same in #19991, but I will copy your test

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

No branches or pull requests

3 participants