-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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: Fixing flake8 errors in cookbook.rst #23837
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great changes, thanks @saurav2608.
I added some comments on things that IMO would make the formatting more consistent and clearer. If you don't mind fixing those.
doc/source/cookbook.rst
Outdated
@@ -105,11 +108,14 @@ Splitting | |||
|
|||
.. ipython:: python | |||
|
|||
df = pd.DataFrame( | |||
{'AAA' : [4,5,6,7], 'BBB' : [10,20,30,40],'CCC' : [100,50,-30,-50]}); df | |||
df = pd.DataFrame({'AAA': [4, 5, 6, 7], 'BBB': [10, 20, 30, 40], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
doc/source/cookbook.rst
Outdated
dflow = df[df.AAA <= 5] | ||
dflow | ||
dfhigh = df[df.AAA > 5] | ||
dfhigh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If those are not being used later, I'd show the result directly, instead of assigning them to a variable first.
doc/source/cookbook.rst
Outdated
def CumRet(x,y): | ||
return x * (1 + y) | ||
def CumRet(x, y): | ||
return x * (1 + y) | ||
|
||
def Red(x): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use red
, I don't think there is a reason to have these functions named as classes
doc/source/cookbook.rst
Outdated
u'beyer': [99, 102, 103, 103, 88, 100]}, | ||
index=[u'Last Gunfighter', u'Last Gunfighter', | ||
u'Last Gunfighter', u'Paynter', u'Paynter', | ||
u'Paynter']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove all the u''
, we'll be supporting Python 3 only very soon.
doc/source/cookbook.rst
Outdated
def gm(aDF,Const): | ||
v = ((((aDF.A+aDF.B)+1).cumprod())-1)*Const | ||
return (aDF.index[0],v.iloc[-1]) | ||
def gm(aDF, Const): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use df
and const
instead?
doc/source/cookbook.rst
Outdated
{u'stratifying_var': np.random.uniform(0, 100, 20), | ||
u'price': np.random.normal(100, 5, 20)}) | ||
{u'stratifying_var': np.random.uniform(0, 100, 20), | ||
u'price': np.random.normal(100, 5, 20)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also can remove the u''
here, and below
doc/source/cookbook.rst
Outdated
{'height': [60, 70], | ||
'weight': [100, 140, 180], | ||
'sex': ['Male', 'Female']}) | ||
{'height': [60, 70], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor thing, but if you want to move this to the previous line, this is how in the previous examples is done.
The CI failed because of If you can, would be nice to run locally after the fixes:
To see that everything is correct, so we can merge. |
Noted your suggestions @datapythonista . I have submitted another PR as this build failed the tests. I will make all these changes and resubmit. I am closing the PR till these are resolved. |
You don't need to open new PR for every change. Just make the changes in the branch you used for the PR, commit and push. The PR will be updated, and it make things much easier for us (and for you I think) this way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks great.
Just one comment. And if you don't mind running flake9-rst doc/source/cookbook.rst
to double check that we didn't miss anything, that would be great.
doc/source/cookbook.rst
Outdated
@@ -533,10 +537,10 @@ Unlike agg, apply's callable is passed a sub-DataFrame which gives you access to | |||
|
|||
S = pd.Series([i / 100.0 for i in range(1, 11)]) | |||
|
|||
def CumRet(x, y): | |||
def cumRet(x, y): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cum_ret
would be the Pythonic way to name the function.
Can you also replace the names in the places where the function names are used below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, added a couple of comments.
doc/source/cookbook.rst
Outdated
@@ -1034,7 +1036,7 @@ You can use the same approach to read all files matching a pattern. Here is an | |||
Finally, this strategy will work with the other ``pd.read_*(...)`` functions described in the :ref:`io docs<io>`. | |||
|
|||
.. ipython:: python | |||
:suppress: | |||
:verbatim: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does :verbatim:
does? I assume the suppress made those not appear in the rendered docs, is that still the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:verbatim: prevents execution as well. In this case I think I have made a mistake. The decorator should remain suppress.
doc/source/cookbook.rst
Outdated
return (aDF.index[0],v.iloc[-1]) | ||
def gm(df, const): | ||
v = ((((df.A + df.B) + 1).cumprod()) - 1) * const | ||
return (v.iloc[-1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return (v.iloc[-1]) | |
return v.iloc[-1] |
doc/source/cookbook.rst
Outdated
@@ -1267,6 +1324,7 @@ The `method` argument within `DataFrame.corr` can accept a callable in addition | |||
... return cov_ab / std_a / std_b | |||
... | |||
... | |||
... | |||
>>> df = pd.DataFrame(np.random.normal(size=(100, 3))) | |||
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't seem like this line is required, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This df is reference in a subsequent line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean't the ...
in the next line, I don't think that is doing anything.
And I think the validation is failing because of the ...
you added before the df = ...
. Can you remove that and see if that makes the CI green?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well here goes. One final try. Thanks for the guidance.
The tests pass on my local. Not sure why automate test is failing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, let's see if the CI ends on green now.
Thanks @saurav2608
Codecov Report
@@ Coverage Diff @@
## master #23837 +/- ##
==========================================
+ Coverage 92.29% 92.29% +<.01%
==========================================
Files 161 161
Lines 51497 51498 +1
==========================================
+ Hits 47528 47530 +2
+ Misses 3969 3968 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @saurav2608, looks great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nits on imports but rest looks fine. Surprised these weren't caught by linter.
Thanks @saurav2608 ! |
git diff upstream/master -u -- "*.py" | flake8 --diff