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

STY: PEP-8 Cleanup #1346

Merged
merged 1 commit into from Jan 29, 2014

Conversation

Projects
None yet
3 participants
@jseabold
Copy link
Member

commented Jan 29, 2014

What to do with one-line style fix commits?

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Jan 29, 2014

merge, ignore or extend to larger cleanup.
The last only if we don't have an open PR on the same code.

@@ -341,7 +341,7 @@ def q_stat(x,nobs, type="ljungbox"):
ret = nobs*(nobs+2)*np.cumsum((1./(nobs-np.arange(1,
len(x)+1)))*x**2)

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Jan 29, 2014

Member

looks like there are a lot more missing spaces in this module

This comment has been minimized.

Copy link
@jseabold

jseabold Jan 29, 2014

Author Member

yeah i'll give it a once over

This comment has been minimized.

Copy link
@jseabold

jseabold Jan 29, 2014

Author Member

pep8 doesn't complain about this. should it?

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Jan 29, 2014

Member

which pep8, the check script? or the newest version of the PEP?
we follow space around operators except power, the PEP does not anymore

This comment has been minimized.

Copy link
@jseabold

jseabold Jan 29, 2014

Author Member

Strike that. It does now.

@coveralls

This comment has been minimized.

Copy link

commented Jan 29, 2014

Coverage Status

Coverage remained the same when pulling f65ef33 on jseabold:pep8cleanup into 48c3376 on statsmodels:master.

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2014

A little more comprehensive now. I ignored whitespace between arithmetic operators in indexing. It's a personal preference.

@@ -790,7 +805,8 @@ def grangercausalitytests(x, maxlag, addconst=True, verbose=True):

if x.shape[0] <= 3 * maxlag + int(addconst):
raise ValueError("Insufficient observations. Maximum allowable "
"lag is {0}".format(int((x.shape[0] - int(addconst)) / 3) - 1))
"lag is {0}".format(int((x.shape[0] - int(addconst)) /
3) - 1))

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Jan 29, 2014

Member

intend to the matching opening bracket?
impossible to see where the 3 belongs

This comment has been minimized.

Copy link
@jseabold

jseabold Jan 29, 2014

Author Member

That's what I had but flake8 complained. I agree it's better though.

This comment has been minimized.

Copy link
@jseabold

jseabold Jan 29, 2014

Author Member

Nm. Fixed.

' df_num=%d') % (fgc1,
stats.f.sf(fgc1, mxlg,
res2djoint.df_resid),
res2djoint.df_resid, mxlg)

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Jan 29, 2014

Member

this zig-zag intend looks "weird"

This comment has been minimized.

Copy link
@jseabold

jseabold Jan 29, 2014

Author Member

Grouped by open parens

@@ -186,24 +191,24 @@ def adfuller(x, maxlag=None, regression="c", autolag='AIC',
if regresults:
store = True

trenddict = {None:'nc', 0:'c', 1:'ct', 2:'ctt'}
trenddict = {None : 'nc', 0 : 'c', 1 : 'ct', 2 : 'ctt'}

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Jan 29, 2014

Member

Is there a rule for this?
ugly in my opinion, or at least I'm not used to it.
I thought dict : is like a keyword = without spaces

This comment has been minimized.

Copy link
@jseabold

jseabold Jan 29, 2014

Author Member

Yes, the rule is no space before ":", which I hate and have proposed to ignore. I have this literally everwhere in my code.

d = {
       key1 : value,
       key2 : value,
      }

I think it's much more readable.

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Jan 29, 2014

Member

I find no spaces around : easier to read (inline), especially in this example where it's more difficult to see what are the pairs.
I agree about the multiline spaces, it always hurts when I do something like this:

d = func(
kwd1=value,
this_is_another_kwd=value,
)

This comment has been minimized.

Copy link
@jseabold

jseabold Jan 29, 2014

Author Member

Ok, I'll fix it, so I can merge this and get back to what I was doing with this file. For the future I still have this rule turned off in my syntax error highlighting.

This comment has been minimized.

Copy link
@jseabold

jseabold Jan 29, 2014

Author Member

Ugh, I don't like the way it looks at all.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Jan 29, 2014

That's what I had but flake8 complained.

for intend in continuation lines that isn't large enough.
I usually try to line up visually (without strict rules), but automatic reformatting, by Kevin's editor for example, messes this up. And I find it hard to read if there is no matching intend.

@coveralls

This comment has been minimized.

Copy link

commented Jan 29, 2014

Coverage Status

Coverage remained the same when pulling 495d546 on jseabold:pep8cleanup into 48c3376 on statsmodels:master.

@@ -186,24 +191,24 @@ def adfuller(x, maxlag=None, regression="c", autolag='AIC',
if regresults:
store = True

trenddict = {None:'nc', 0:'c', 1:'ct', 2:'ctt'}
trenddict = {None: 'nc', 0: 'c', 1: 'ct', 2: 'ctt'}

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Jan 29, 2014

Member

I think that looks good,
the "stranger" thing was the space before the :

jseabold added a commit that referenced this pull request Jan 29, 2014

@jseabold jseabold merged commit 541d746 into statsmodels:master Jan 29, 2014

1 check passed

default The Travis CI build passed
Details

@jseabold jseabold deleted the jseabold:pep8cleanup branch Jan 29, 2014

PierreBdR pushed a commit to PierreBdR/statsmodels that referenced this pull request Sep 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.