Fix stylistic issues in HARK

[shaunagm]

We've adapted flake8 for "code linting", as per issue #171. Now we need to use flake8's output to make our code more pythonic. This is a great issue for newcomers who want to get familiar with our codebase.

To run flake8, make sure you've installed the requirements in requirements.txt (or simply type pip install flake8) and then run in your command line/prompt flake8 HARK/core.py, substituting the relative path is to the file you want to lint.

I've broken out the codebase into sections so multiple people can tackle this issue at once. If you'd like to grab a section, comment below.

• [ ] top level HARK folder (core.py; dcegm.py; estimation.py; interpolation.py; parallel.py; simulation.py; utilities.py
• [ ] tests folder
• [ ] ConsumptionSaving folder
• [ ] FashionVictim folder
• [ ] SolvingMicroDSOPs folder
• [ ] cAndCwithStickyE folder
• [ ] cstwMPC folder

[StephenSchroeder]

I was thinking I will start off by linting the tests folder!

[rsaavy]

I will be working on pylinting on core.py

EDIT* Core.py has been linted.

[StephenSchroeder]

I just finished linting the Testing folder, and I will continue linting the tests folder (within the HARK folder which is probably where the linting was meant to be done anyways)

[llorracc]

Awesome, thanks! I guess @shaunagm probably should be the one to merge the PR's since she is on the spot (and knows more about this than we do anyway!)

[rsaavy]

I'm working on dcegm.py

I ran into this error for flake8

dcegm.py:155:25: E712 comparison to False should be 'if cond is False:' or 'if not cond:'

[llorracc]

Patrick has probably gone to bed (I think it's midnight now in Copenhagen) so he can look at this tomorrow.

(I'm new to flake8. My guess about that message was that it referred to what it thought was a problem on line 155 (not sure what the '25' was), but line 155 is upperV_T = np.zeros(m_len) which does not seem related. Maybe it is actually identifying line 158, which reads:

upperV_T[row_all_nan == False] = np.nanmax(commonV_T[row_all_nan == False, :], axis=1) ?

[rsaavy]

Hi,

Line 155 for me after I had removed some lines is idx_max[row_all_nan == False] = np.nanargmax(commonV_T[row_all_nan == False], axis=1)

[llorracc]

Hmm, so do you think it is suggesting

upperV_T[if row_all_nan is False] = np.nanmax(commonV_T[if row_all_nan is False, :], axis=1)

? That looks odd to me.

[rsaavy]

I've linted most of it the smaller issues:

The errors dcegm.py:155:25: E712 comparison to False should be 'if cond is False:' or 'if not cond:' dcegm.py:161:26: E712 comparison to False should be 'if cond is False:' or 'if not cond:' dcegm.py:175:50: E712 comparison to False should be 'if cond is False:' or 'if not cond:'

I will make PR so you can see which lines those are

[shaunagm]

So it seems to be complaining that we're confusing "falsy" with "false". In Python, "falsy" values are any values where bool(value) returns False, for instance False, [] and None are falsy, while only False is false. "If not cond" is saying "if cond is falsy", whereas "if cond is False" is saying "if cond is False". It wants us to determine if cond should be falsy or false. Can it be a [ ] or a None or can it only be False?

[llorracc]

OK, that makes sense. But I don't know the answer -- we'll have to wait for @pkofod

[mnwhite]

It looks to me like we can't use "cond is False", because cond is a (boolean) array, not just a single bool. You definitely can't put an if statement inside indexing brackets. I would write this line as:

valid_rows = np.logical_not(row_all_nan) upperV_T[ valid_rows ] = np.nanmax(commonV_T[ valid_rows , :], axis=1)

On Mon, May 6, 2019 at 5:00 PM Christopher Llorracc Carroll < notifications@github.com> wrote:

OK, that makes sense. But I don't know the answer -- we'll have to wait for @pkofod https://github.com/pkofod

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/262#issuecomment-489774734, or mute the thread https://github.com/notifications/unsubscribe-auth/ADKRAFM5PJQ5VDI72DK6E2DPUCL5NANCNFSM4HI4W2FQ .

[shaunagm]

FYI Ingrid & I are grabbing estimation.py to lint.

We are on to parallel.py.

[michiboo]

Hi I will take SolvingMicroDSOPs folder

[sbenthall]

Since this issue has been made, we've converged now on black for linting/styling.

I wonder if we should make run this as the final step before the 0.10.7 release. The problem with this change is that because it touches all the files, it's hard to do when there's a lot of ongoing work in PRs.

What do you think, @MridulS ?

I think we may also be waiting for more definite buy-in from @llorracc and @mnwhite on this.

[MridulS]

We use black now. :)