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

STYLE: Fix miscellaneous Python warnings #2606

Merged
merged 7 commits into from May 29, 2022

Conversation

jhlegarreta
Copy link
Contributor

@jhlegarreta jhlegarreta commented May 28, 2022

  • DOC: Use triple double-quoted strings in docstrings
  • STYLE: Avoid array-like mutable default argument values
  • STYLE: Avoid re-declaring variables in nested for loops
  • STYLE: Replace function call with set literal
  • STYLE: Simplify chained comparisons
  • STYLE: Avoid shadowing built-in names

Conform to PEP 257: use triple double-quoted strings in docstrings.

Fixes:
```
Triple double-quoted strings should be used for docstrings.
```
Avoid array-like mutable default argument values.

Fixes:
```
Default argument value is mutable
```

Change the docstrings accordingly.
Remove redundant parentheses.

Fixes:
```
Remove redundant parentheses
```
Avoid re-declaring variables in nested `for` loops.

Fixes e.g.:
```
Variable 'dim' is already declared in 'for' loop or 'with' statement above
```
Replace function call with set literal.

Fixes:
```
Function call can be replaced with set literal
```
@pep8speaks
Copy link

pep8speaks commented May 28, 2022

Hello @jhlegarreta, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2022-05-29 19:17:47 UTC

@jhlegarreta jhlegarreta force-pushed the FixMiscPythonWarnings2 branch 2 times, most recently from 8606dcc to ac4b478 Compare May 29, 2022 01:24
@@ -679,7 +679,7 @@ def l_shore(radial_order):


def n_shore(radial_order):
"Returns the angular regularisation matrix for SHORE basis"
"""Returns the angular regularisation matrix for SHORE basis"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, the l_shore, and n_shore methods are exactly the same. I have not found the equations in the paper. If they should be the same, then only one method should exist.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe @maurozucchelli has an answer concerning l_shore and n_shore?

@codecov
Copy link

codecov bot commented May 29, 2022

Codecov Report

Merging #2606 (cc31f08) into master (ebd1bf5) will decrease coverage by 0.00%.
The diff coverage is 50.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2606      +/-   ##
==========================================
- Coverage   84.27%   84.26%   -0.01%     
==========================================
  Files         130      130              
  Lines       17914    17916       +2     
  Branches     3051     3051              
==========================================
  Hits        15097    15097              
- Misses       2102     2104       +2     
  Partials      715      715              
Impacted Files Coverage Δ
dipy/reconst/forecast.py 90.04% <0.00%> (ø)
dipy/reconst/shore.py 90.03% <ø> (ø)
dipy/viz/panel.py 51.86% <ø> (ø)
dipy/workflows/viz.py 62.96% <0.00%> (ø)
dipy/viz/plotting.py 7.40% <3.92%> (-0.19%) ⬇️
dipy/io/bvectxt.py 37.50% <50.00%> (ø)
dipy/core/optimize.py 76.33% <51.89%> (ø)
dipy/core/gradients.py 86.81% <58.33%> (ø)
dipy/reconst/mapmri.py 89.66% <71.01%> (ø)
dipy/data/__init__.py 78.76% <73.68%> (ø)
... and 8 more

@jhlegarreta
Copy link
Contributor Author

Failing build is codecov/patch; this PR does not modify the coverage, so the failure can be ignored.

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @jhlegarreta,

Thank you for this PR. It is almost ready to go. See below for some comments.

dipy/align/parzenhist.pyx Outdated Show resolved Hide resolved
dipy/align/parzenhist.pyx Outdated Show resolved Hide resolved
dipy/align/parzenhist.pyx Outdated Show resolved Hide resolved
@@ -679,7 +679,7 @@ def l_shore(radial_order):


def n_shore(radial_order):
"Returns the angular regularisation matrix for SHORE basis"
"""Returns the angular regularisation matrix for SHORE basis"""
Copy link
Member

Choose a reason for hiding this comment

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

Maybe @maurozucchelli has an answer concerning l_shore and n_shore?

@jhlegarreta
Copy link
Contributor Author

Same failure as for the previous commit, codecov, false positive.

Simplify chained comparisons.

Fixes:
```
Too complex chained comparisons.
```
Rename method parameters and local variables to avoid shadowing built-in
names.

Fixes e.g.
```
Shadows built-in name 'bytes'
```
@skoudoro skoudoro merged commit 12f3324 into dipy:master May 29, 2022
@skoudoro
Copy link
Member

LGTM ! Thank you @jhlegarreta

@jhlegarreta jhlegarreta deleted the FixMiscPythonWarnings2 branch May 29, 2022 22:22
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.

None yet

3 participants