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

LGTM code quality suggestions #588

Merged
merged 5 commits into from
Nov 11, 2021
Merged

Conversation

nshea3
Copy link
Contributor

@nshea3 nshea3 commented Oct 2, 2021

Hi all!

This PR addresses the code quality alerts in #466

Three different things that LGTM picked up in the repo:

  1. An import * in quantecon/game_theory/init.py. This one was a relatively straightforward fix, just updated with the specific imports required.
  2. "wrong number of arguments" in quantecon/util/timing.py. I think this one is a false alarm - there is branching logic to distinguish between the no argument, one argument, and many argument cases. Triggered by a call in TestTicTacToc.test_loop.test_function_one_arg. Have not made any changes to this code since I believe LGTM is mistaken here.
  3. Module (numpy) is imported with 'import' and 'import from' in a couple different files. This is triggered by an import numpy as np followed by from numpy import dot etc. which is pretty reasonable especially with big formulas with lots of linear algebra. I see two possible approaches here:
    • Replace all dot(A,B) calls with np.dot(A,B)
    • Replace all dot(A,B) calls with A.dot(B)

For completeness here I defaulted to the first approach since it's a one-to-one drop in replacement and won't require any significant rewrites. However it does not help with readability or brevity, so I can certainly drop those commits from the PR if need be.
Happy to give the second approach a try as well.

LGTM list for reference: https://lgtm.com/projects/g/QuantEcon/QuantEcon.py/alerts?mode=list

Looking forward to your feedback on this! Thanks,

Nick

@pep8speaks
Copy link

Hello @nshea3! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 250:80: E501 line too long (81 > 79 characters)

Line 140:14: E127 continuation line over-indented for visual indent
Line 142:14: E127 continuation line over-indented for visual indent

Line 190:9: E741 ambiguous variable name 'I'
Line 203:80: E501 line too long (80 > 79 characters)

@jstac
Copy link
Contributor

jstac commented Oct 2, 2021

Thanks @nshea3 , we appreciate the contribution!

Actually my preference is that all of those np.dot calls are replaced by @ syntax, which makes the matrix algebra more readable. Some of that code was written before @ was implemented as matrix multiplication.

I'll let the maintainers (@mmcky and @oyamad) weigh in as well.

@mmcky
Copy link
Contributor

mmcky commented Oct 31, 2021

thanks @nshea3 these look like good changes. Sorry for my delay on this.

I like @jstac suggestion of switching over to using @ instead of np.dot. But happy for us to merge these LGTM code quality suggestions and then migrate np.dot to @ in another PR.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 94.452% when pulling c6902d7 on nshea3:lgtm-code-quality into 7b6b4a7 on QuantEcon:master.

@nshea3
Copy link
Contributor Author

nshea3 commented Nov 10, 2021

Hi @mmcky ! No worries!
I have explored switching over to the @ instead of np.dot. However that is going to be a little more involved - it's not a drop-in replacement since it is much more particular about the datatypes and matrix dimensions it will accept as arguments. I'll open up a separate issue and PR to discuss those changes.

Happy to merge this in for the time being or close without merging, whichever seems best to you and @jstac

@mmcky
Copy link
Contributor

mmcky commented Nov 11, 2021

thanks @nshea3 -- Thanks for looking into the @ migration. This explanation is helpful when thinking about the differences

https://stackoverflow.com/questions/34142485/difference-between-numpy-dot-and-python-3-5-matrix-multiplication

Once an issue is open I will go ahead and merge these improvements and then we can loop back around with @ operator.

@mmcky
Copy link
Contributor

mmcky commented Nov 11, 2021

@nshea3 I have opened this issue

#589

@mmcky
Copy link
Contributor

mmcky commented Nov 11, 2021

thanks @nshea3 for these changes in code quality. Greatly appreciated.

@mmcky mmcky merged commit b23e43d into QuantEcon:master Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants