Lint codebase with additional style restrictions

[tsalo]

Closes None. I'll probably tackle the style issues the new extensions catch after a few of the other PRs have been merged.

Changes proposed in this pull request:

• Add more flake8 extensions.
  • flake8-absolute-import: Enforces absolute imports.
  • flake8-docstrings: Enforces numpydoc-style docstrings.
  • flake8-unused-arguments: Checks for unused arguments in classes and functions.
  • flake8-use-fstring: Checks that we use modern f-strings over .format() and %.
  • pep8-naming: Enforces PEP8-compliant variables, classes, and functions.

[eurunuela]

Hey @tsalo, do you know if there's a way in flake8 to set the number of summary lines to 2?

With the pre-commit I added to this PR, we could get rid of most docstring issues related to summary lines and trailing periods.

If it isn't possible, I would ignore the D205 1 blank line required between summary line and description error.

[handwerkerd]

If I understand correctly, flake8 vs rust should not change how the code is currently being linted. If we can get this working, I'd recommend merging and then consider rust. I really want this merged because it's going to affect everything else I'm working on and the sooner I can deal with the changes this creates, the better.

I don't think the speed boost matters to us so I'm not going to advocate for a change to rust, but I'm not opposed.

[handwerkerd]

I also took a look at this since all the tests have now passed. I tried to make html for the docs and I'm seeing a bunch of problems.

/Users/handwerkerd/code/meica/tedana/tedana/decomposition/pca.py:docstring of tedana.decomposition.pca.tedpca:39: WARNING: Block quote ends without a blank line; unexpected unindent.
/Users/handwerkerd/code/meica/tedana/tedana/decomposition/pca.py:docstring of tedana.decomposition.pca.tedpca:41: WARNING: Block quote ends without a blank line; unexpected unindent.

In the same file, the following is now causing a LaTeX rendering failure

            .. math::
                {\\kappa}_c = \\frac{\\sum_{v}^V {\\zeta}_{c,v}^p * \
                      F_{c,v,R_2^*}}{\\sum {\\zeta}_{c,v}^p}

                {\\rho}_c = \\frac{\\sum_{v}^V {\\zeta}_{c,v}^p * \
                      F_{c,v,S_0}}{\\sum {\\zeta}_{c,v}^p}

[eurunuela]

That's odd... those lines have not been edited in 4 years...

I will have a look and see if I can fix it.

[eurunuela]

I have just fixed the issue with LaTeX.

7907c39 prepended an r to the docstring, which made the LaTeX fail because it contained double slashes. Apparently, either you use double slashes (e.g., \\lambda), or you use a single slash and prepend the r to the docstring.

[eurunuela]

Okay, I see now why the r was added. It was one of the new flake8 requirements.

I have opted for using a single slash with the r.

Edit: I think we can merge this if you're happy with the changes @handwerkerd. I assume @tsalo is also happy with the changes?

[handwerkerd]

Found another place with double slashes that need to be changed to single. I'm looking through the rest of this now.

https://github.com/tsalo/tedana/blob/e194bacc1c449790b19f432d56ac237abeb16b45/tedana/combine.py#L139-L141

[eurunuela]

Found another place with double slashes that need to be changed to single. I'm looking through the rest of this now.

https://github.com/tsalo/tedana/blob/e194bacc1c449790b19f432d56ac237abeb16b45/tedana/combine.py#L139-L141

Good catch! I must've missed it earlier. I think that should be all of them.

[handwerkerd]

I've scanned all the changes and skimmed the docs to see if there were any unintended changes. I found a few minor things.

The one non-trivial thing, which I've commented in a few places is that we used a lot of capitalized acronyms in variable names T2 S0 and SSE. I'd like to keep these capitalized, but I assume the specification requires variables to use lowercase letters. If this is a headache to change back or others have a strong opinion on why they should all be lowercase (i.e. no accidental case differences in variable names) I'm fine keeping all lowercase.

[handwerkerd]

Interesting that combmode and t2sG are no longer used. They were only useful for the kundu options and my best guess is that they stopped being used here when metrics.collect.generate_metrics was added. The one work is that those are doe within generate_metrics, but the option for combmode can no longer be externally set. I don't think this a problem or needs correction, but figured I'd highlight in case others see an issue.

[handwerkerd]

I think the plan was to merge this without it being counted in the tallies of lines-of-code changed. Is that properly set up?

[handwerkerd]

FWIW, this is really close to merging. I made a few minor suggestions, but I hope they don't hold up the merge. I'd love to be able to update a few of the other PRs with this one before working on them more.

[eurunuela]

I don't know how we can keep the capitalized acronyms and still use the pep8-naming convention... @tsalo do you know if there is a way?

In any case, while I understand why we would want to keep them capitalized, it doesn't really bother me that they get changed to lowercase.

[tsalo]

I don't know how we can keep the capitalized acronyms and still use the pep8-naming convention... @tsalo do you know if there is a way?

Not that I know of. Per PEP8, uppercase characters are limited to classes, rather than functions or variables (except constants). That said, I don't think we consistently refer to acronyms in uppercase throughout the package (e.g., t2s is lowercase in many cases), so I don't think it's an issue to make them lowercase. If there are cases where the lowercase version is less clear (e.g., f_t2), I think we can improve the variable names (e.g., fstat_t2).

[eurunuela]

Since this is a PR that I believe you need merged @handwerkerd, shall we open an issue to rename the variables that are now lowercase? Or would you rather take care of that in this PR?

[handwerkerd]

Just keep the variables lowercase. I made a suggestion, but I'm fine keeping them as is in this PR.

[eurunuela]

Just keep the variables lowercase. I made a suggestion, but I'm fine keeping them as is in this PR.

I believe I already added your suggestion if you're referring to 964b0f6.

Edit: if that's the case, feel free to merge.

[handwerkerd]

I just tried to update one of the branches I was working on with the newly linted code with the pre-commit hooks. I'm seeing two weird things.

First, when I merged the PR and tried to commit the branch, it made a few more changes in tests. It was all for cases like the following where it tried to add an additional period after the first line of the doc string:

tedana/tedana/tests/test_component_selector.py

Lines 197 to 198 in b139386

	Tests to make sure validate_tree suceeds for all default
	decision trees in decision trees.

I'm suspecting the pre-commit hooks are using a slightly different standard on my local version and trying to push this added changes. Any ideas on how to fix my local settings?

The second is weirder and I'm hoping it resolves itself. In trying to show the above issue, I tried to compare branches:
https://github.com/ME-ICA/tedana/compare/ME-ICA:tedana:main...handwerkerd:tedana:doc-tree-dh?expand=1
It looks to me like it's trying to compare my branch with main before this PR was merged. Maybe whatever was done to make sure style changes weren't counted in line counts also messed up comparing branches?