STYLE: Format code using ruff
#3216
Conversation
jhlegarreta
force-pushed
the
ApplyRuffFormatting
branch
19 times, most recently
from
cab137f
to
849d75a
Compare
|
@skoudoro Despite all CIs being failing, this is working, tests are passing, e.g.: Test failures (warnings treated as errors) are unrelated: In fact, the Not sure this has only appeared yesterday. But anyways, better to address it in a separate PR. Edit: PR #3217. Some comments:
|
jhlegarreta
force-pushed
the
ApplyRuffFormatting
branch
from
849d75a
to
a71d150
Compare
|
Hi @jhlegarreta, Thank you for this huge work, I will go through it during the day. Can you address the conflict and rebase or merge this PR with master since the cvxpy warning has been fixed. Thank you |
jhlegarreta
force-pushed
the
ApplyRuffFormatting
branch
from
a71d150
to
7c87b5d
Compare
Done. |
| def __init__( | ||
| self, | ||
| X, | ||
| Y, | ||
| sigma2=None, | ||
| alpha=None, | ||
| beta=None, | ||
| low_rank=False, | ||
| num_eig=100, | ||
| max_iterations=None, | ||
| tolerance=None, | ||
| w=None, | ||
| *args, | ||
| **kwargs, | ||
| ): |
There was a problem hiding this comment.
can we change this rule for function ? This take too much space for no reason...
What do you think? What is your opinion @arokem also ?
I know that @Garyfallidis prefer too maximize the line-length instead of 1 argument per line.
There was a problem hiding this comment.
can we change this rule for function ?
I'd say that this is determined by the line length: I am not aware of a setting that tells ruff: "add as many arguments as possible on the same line until the line length, and break the line only then". If you happen to find it, please let me know and I'll adapt it if Ariel and Elef are against the current style.
I know it is hard to read sometimes, and it requires effort to get used to it. This is one of prices to pay for the automation/consistency.
There was a problem hiding this comment.
@skoudoro @Garyfallidis @arokem after rebasing the failures in code-format are again the expected ones (#3216 (comment)) so if CI's come green this should be considered for merging. Rebasing a PR with 4 commits changing 33K line is hard and very time consuming, also because changes need to be done to the first commit, and at times I forget. Also, note that I had to make >400 manual changes because ruff was finding statements that did not comply strictly with PEP8. So I'd be grateful if you could afford some time to consider this PR.
There was a problem hiding this comment.
ok, I am going ahead and merge. but many formatting are still strange to me like my comment above and many array formatting. we might need use fmt off and on for the array.
Many new PRs will be needed to finalize/adapt this work
There was a problem hiding this comment.
also, the correction for SimilarityMetric deriving from abc.ABCMeta seems incorrect following: https://docs.astral.sh/ruff/rules/meta-class-abc-meta/
There was a problem hiding this comment.
Concerning import sorting, I got my answer in the links below:
astral-sh/ruff#2600
https://docs.astral.sh/ruff/settings/#lint_isort_split-on-trailing-comma
There was a problem hiding this comment.
we might need use fmt off and on for the array.
Seems not very practical to me, and leads back to manual formatting/inconsistency. But feel free to have a go.
I will have a look at #3216 (comment).
There was a problem hiding this comment.
OK, had a look at the ABC inheritance. Unless other changes are made, a simple class C(ABC): will not do the job: you will then get
dipy/align/metrics.py:97:5: B027 `SimilarityMetric.use_static_image_dynamics` is an empty method in an abstract base class, but has no abstract decorator
dipy/align/metrics.py:136:5: B027 `SimilarityMetric.use_moving_image_dynamics` is an empty method in an abstract base class, but has no abstract decorator
and if you add the decorator to those functions, the CI's start failing, so either the methods need to be reimplemented in child classes with a simple pass or I do not see a way to inherit from ABC. @skoudoro feel free to have a stab at this.
jhlegarreta
force-pushed
the
ApplyRuffFormatting
branch
2 times, most recently
from
7c2a24b
to
49ec4ec
Compare
Format the code using `ruff`: supersedes `flake8`. Set the pep8speaks `max-line-length` property to 88 as set in `ruff.toml` after internal discussion/agreement. Bump `ruff-pre-commit` pre-commit hook version to v0.4.3 (May 3, 2024): the version being used previously was v0.1.7 was from Dec 4, 2023. Sort the imports according to the new version: new sorting is dictated by the default value of `force-sort-within-sections` (`false`): imports are sorted by module. Fix the warnings reported by `ruff`, e.g. C408, F401, etc. Add `laf` (left arcuate fasciculus) bundle tractogram filename to the `codespell` exception list. Adapt the DIPY coding style guideline to reflect the use of the `pre-commit` and `ruff`-based style enforcement framework.
Remove expression that has no effect. Fixes: ``` dipy/reconst/dti.py:1378:21: B018 Found useless expression. Either assign it to a variable or remove it. ```
Do not derive `SimilarityMetric` from `abc.ABCMeta`: it is not an abstrac class as not all of its child classes implement all of its methods. Fixes: ``` dipy/align/metrics.py:97:5: B027 `SimilarityMetric.use_static_image_dynamics` is an empty method in an abstract base class, but has no abstract decorator dipy/align/metrics.py:136:5: B B027 `SimilarityMetric.use_moving_image_dynamics` is an empty method in an abstract base class, but has no abstract decorator ```
Remove cast to deprecated `long` type: `long` was renamed to `int` in Python 3.0, and DIPY does not support Python 2 since May 7, 2019 (DIPY 1.0.0). Fixes: ``` dipy/segment/tests/test_feature.py:316:20: F821 Undefined name `long` ``` Documentation: https://peps.python.org/pep-0237/ https://docs.python.org/3.0/whatsnew/3.0.html#integers http://python3porting.com/differences.html#long
jhlegarreta
force-pushed
the
ApplyRuffFormatting
branch
from
49ec4ec
to
8e0733f
Compare
ruffSimilarityMetricfromabc.ABCMetalongtype