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

DOC: enable docstring on DataFrame.columns/index #20385

Merged
merged 4 commits into from
Mar 22, 2018

Conversation

jorisvandenbossche
Copy link
Member

Follow-up on #18196, similar to #19991

Didn't add an actual full docstring yet, but this already gives:

In [1]: pd.DataFrame.columns?
Type:        AxisProperty
String form: <pandas._libs.properties.AxisProperty object at 0x7fa975d74da0>
File:        ~/scipy/pandas/pandas/_libs/properties.cpython-35m-x86_64-linux-gnu.so
Docstring:   columns

instead of (on master, on 0.22 this raises an exception):

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

Py_ssize_t axis

def __init__(self, axis=0):
cdef readonly:
object __doc__
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this go under the same def readonly block as axis?

And do we want object or str for the type?

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops, the above cdef readonly is a typo, I meant to keep it as readonly.
I am not fully sure if it is actually needed for axis, or if we can also make it readonly (I would think it is never modified internally, bot not sure)

For the object, it is what is done for CachedProperties above as well. My cython is not good enough to really know the consequences. I think we usually use object for strings in our cython code.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the test pass with using readonly for axis, I will combine them in one cdef readonly

Copy link
Member

Choose a reason for hiding this comment

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

object vs str is pretty harmless in this case. I'd advocate sticking with object for now b/c then we don't need to worry about potential py2/py3 bytes/unicode corner cases.

@codecov
Copy link

codecov bot commented Mar 17, 2018

Codecov Report

Merging #20385 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #20385      +/-   ##
==========================================
+ Coverage   91.77%    91.8%   +0.02%     
==========================================
  Files         152      152              
  Lines       49205    49215      +10     
==========================================
+ Hits        45159    45181      +22     
+ Misses       4046     4034      -12
Flag Coverage Δ
#multiple 90.18% <100%> (+0.02%) ⬆️
#single 41.84% <100%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/panel.py 97.29% <ø> (ø) ⬆️
pandas/core/series.py 93.84% <ø> (ø) ⬆️
pandas/core/frame.py 97.18% <ø> (ø) ⬆️
pandas/core/generic.py 95.85% <100%> (ø) ⬆️
pandas/core/window.py 96.26% <0%> (-0.01%) ⬇️
pandas/plotting/_core.py 82.27% <0%> (ø) ⬆️
pandas/io/json/normalize.py 96.93% <0%> (+0.06%) ⬆️
pandas/util/testing.py 84.11% <0%> (+0.16%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7273ea0...c9a8a34. Read the comment docs.

@@ -278,7 +278,7 @@ def _setup_axes(cls, axes, info_axis=None, stat_axis=None, aliases=None,
if build_axes:

def set_axis(a, i):
setattr(cls, a, properties.AxisProperty(i))
setattr(cls, a, properties.AxisProperty(i, a))
Copy link
Member

Choose a reason for hiding this comment

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

This setattr has always bothered me. It kind of makes since in a general-case world with PanelND, but if we're only interested in Series/DataFrame/[barely-]Panel wouldn't it be clearer just to write:

class DataFrame(NDFrame):
    index = properties.AxisProperty(0)
    columns = properties.AxisProperty(1)

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, fully agree that if we only have 1D/2D this makes much more sense. But maybe we should only clean that up when Panel is removed?

@jorisvandenbossche jorisvandenbossche merged commit 8f9ffcc into pandas-dev:master Mar 22, 2018
@jorisvandenbossche jorisvandenbossche deleted the doc-axes branch March 22, 2018 09:46
javadnoorb pushed a commit to javadnoorb/pandas that referenced this pull request Mar 29, 2018
dworvos pushed a commit to dworvos/pandas that referenced this pull request Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants