Automated code quality improvements

[spxiwh]

ruff the new linter recently added has some tools for automatically changing code to match the formatting settings. These tools are "safe" in the sense that they don't/can't change functionality. This PR is the result of running that. We also extend the qlty configuration file such that we are picking up the files in bin (for the first time).

Predictably, this is a lot of files! I don't expect anybody to look over each of these (though you can get a sense of what it is doing). I would be concerned about the issue of needing to rebase other ongoing work due to conflicts if people have edited code that this is touching.

Suggestions for what to do with this? ... There are still quite a few remaining things to address in the codebase in terms of code quality, but I might discuss that in a separate issue if this is okay to merge.

Standard information about the request

This is a: Code Quality PR

This change affects: No functionality.

This change changes: Code quality

This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines

Motivation

Wanting to improve code quality and ruff offers automated tools to do this.

Contents

Many files edited

Links to any issues or associated PRs

N/A

Testing performed

We'll have to rely on the test CI for this.

• The author of this pull request confirms they will adhere to the code of conduct

[github-advanced-security]

qlty found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[GarethCabournDavies]

This is finding some nice little bits to check up on! The zip() strict thing seems really nice and useful, I have definitely fallen into that trap before

[GarethCabournDavies]

On the /bin/ files, there was a back burner thought that we should maybe move to the model where everything is a python script, with a def main() in each tool which can be defined as an executable in pyproject.toml. Would that now be superfluous?

[spxiwh]

The linter is setup to run on files being edited (with much weaker rules than what I'm running locally, or used to generate this PR). It's therefore picking up on a bunch of pre-existing issues (especially in the bin files, which haven't been checked until now). In short, the linter is definitely not going to pass here! However, it highlights that a bunch of low-hanging things to fix after this remains.

We still ship binaries in a now nonstandard way, this doesn't change that, it just lets us run the code quality checks on the executables.

In terms of rough breakdown of issues that this fixes:

Rule	Name	pycbc/ commit	bin/ commit	Total
Q000	bad-quotes-inline-string	8895	–	8895
D213	multi-line-summary-second-line	1973	75	2048
D413	missing-blank-line-after-last-section	1327	16	1343
D210	surrounding-whitespace	633	–	633
I001	unsorted-imports	294	287	581
UP032	f-string	270	193	463
D208	over-indentation	359	–	359
D200	unnecessary-multiline-docstring	357	–	357
D409	mismatched-section-underline-length	332	2	334
E501	line-too-long	218	–	218
D202	blank-line-after-function	184	29	213
D204	incorrect-blank-line-after-class	171	–	171
UP008	super-call-with-parameters	154	1	155
RET505	superfluous-else-return	140	12	152
W293	blank-line-with-whitespace	108	–	108
UP004	useless-object-inheritance	88	16	104
W291	trailing-whitespace	94	–	94
D300	triple-single-quotes	67	14	81
E401	multiple-imports-on-one-line	22	58	80
D411	no-blank-line-before-section	64	1	65
Q002	bad-quotes-docstring	60	–	60
UP030	format-literals	23	35	58
PIE790	unnecessary-placeholder	56	2	58
D209	new-line-after-last-paragraph	45	11	56
D410	no-blank-line-after-section	46	–	46
D214	overindented-section	43	–	43
D215	overindented-section-underline	43	–	43
SIM118	in-dict-keys	25	17	42
ISC003	explicit-string-concatenation	8	26	34
G001	logging-string-format	15	19	34
PLR1730	if-stmt-min-max	18	15	33
RUF022	unsorted-dunder-all	28	–	28
RET506	superfluous-else-raise	25	–	25
PLR5501	collapsible-else-if	17	6	23
UP015	redundant-open-modes	13	9	22
W605	invalid-escape-sequence	–	21	21
B010	set-attr-with-constant	20	–	20
UP024	os-error-alias	13	7	20
D403	first-word-uncapitalized	15	2	17
D412	blank-lines-between-header-and-content	16	–	16
C420	unnecessary-dict-comprehension-for-iterable	8	8	16
D211	blank-line-before-class	14	–	14
E701	multiple-statements-on-one-line-colon	13	–	13
D205	missing-blank-line-after-summary	13	–	13
E713	not-in-test	5	7	12
UP034	extraneous-parentheses	7	4	11
PLR1711	useless-return	5	6	11
...and 62 more codes, each ≤ 9

[spxiwh]

The failure is not related to this, and seems to be an issue with emcee being incompatible with latest numpy.

[tdent]

What is zip strict ?

[GarethCabournDavies]

What is zip strict ?

zip strict enforces that both things being zipped are the same length

[titodalcanton]

I worry this would make it essentially impossible to backport PRs to existing active release branches (e.g. 2.11), though that could be hopefully addressed by merging a PR equivalent to this into the release branch first.

[tdent]

With 507 files changed, how can we isolate actual changes in code (and possibly learn from them ..) rather than formatting conventions, etc.?

[titodalcanton]

Would it help to identify all the files that are not being touched by any currently open PRs, and apply the change only to those as a first step? Then, as the remaining PRs get merged over time, we can periodically go through this exercise again and slowly reformat all the remaining files. Too complicated?

[spxiwh]

@tdent This PR is only formatting changes. There are some "actual changes in code" that would follow this, but I wanted to put this out first to gauge appetite for trying to improve "code quality" (and because following patches require a human to actually check them, this is advertised as "safe")

[spxiwh]

The list I printed earlier is the full list of things being fixed here, if anyone wanted to learn from this, but the idea would be that once this is merged future PRs would complain if any of these issues are reintroduced.

(I note though, that the majority of recent PRs are done using AI agents, in which case the agent is just fixing this and the user doesn't have to learn or care)

[spxiwh]

@titodalcanton I could try and exclude files being edited, but we have 62 open PRs, so how far back do I go? ... And it still doesn't fix the issue of the PR that someone's about to put up or the issue of cherry-picking later commits onto diverging branches .... I note that for anyone writing PRs with coding agents (which is now very common) dealing with this will be easy though.

[tdent]

The big table you pasted earlier has a lot of formatting fixes, but also what I would consider code changes, although they may be guaranteed to not change functionality: e.g. if-stmt-min-max is a code change, and certainly inserting strict=True in all our zip()'s is a code change.

[spxiwh]

So I would argue that https://docs.astral.sh/ruff/rules/if-stmt-min-max/ is not a code change, it's just formatting. It is a safe code change though in that it's still doing the same thing.

The zip thing is a genuine code change, and is not included here.

[spxiwh]

But again, I can split some of these off as desired, but one commit per each of these issues would be worse than doing this in one block (both for me and for rebasing changes)

[spxiwh]

The leading letter is indicative of the type of fix, so perhaps splitting this off by letter code would be one approach. e.g. "Q" type fixes, "D" type fixes etc.

[titodalcanton]

I am leaning towards accepting to merge this now, however could you please indicate exactly what command you ran to do this? We will need that reference for doing the same thing to active release branches.

[tdent]

Ah, then don't know why Gareth mentioned zip(strict=True) earlier. The table of different codes/types of changes is incomplete (it says 62 more not displayed), so I don't know, if they were visible, whether there would be anything I would consider a code change in terms of. Is it possible to show the entire list of codes?

[GarethCabournDavies]

Ah, then don't know why Gareth mentioned zip(strict=True) earlier

Sorry for the confusion, I mistakenly thought that this was what is being fixed, it's actually what is coming up from the reviewer bot (e.g. here)

[spxiwh]

Here's the full set of things fixed:

Rule	Name	pycbc/ commit	bin/ commit	Total
Q000	bad-quotes-inline-string	8895	-	8895
D213	multi-line-summary-second-line	1973	75	2048
D413	missing-blank-line-after-last-section	1327	16	1343
D210	surrounding-whitespace	633	-	633
I001	unsorted-imports	294	287	581
UP032	f-string	270	193	463
D208	over-indentation	359	-	359
D200	unnecessary-multiline-docstring	357	-	357
D409	mismatched-section-underline-length	332	2	334
E501	line-too-long	218	-	218
D202	blank-line-after-function	184	29	213
D204	incorrect-blank-line-after-class	171	-	171
UP008	super-call-with-parameters	154	1	155
RET505	superfluous-else-return	140	12	152
W293	blank-line-with-whitespace	108	-	108
UP004	useless-object-inheritance	88	16	104
W291	trailing-whitespace	94	-	94
D300	triple-single-quotes	67	14	81
E401	multiple-imports-on-one-line	22	58	80
D411	no-blank-line-before-section	64	1	65
Q002	bad-quotes-docstring	60	-	60
PIE790	unnecessary-placeholder	56	2	58
UP030	format-literals	23	35	58
D209	new-line-after-last-paragraph	45	11	56
D410	no-blank-line-after-section	46	-	46
D214	overindented-section	43	-	43
D215	overindented-section-underline	43	-	43
SIM118	in-dict-keys	25	17	42
ISC003	explicit-string-concatenation	8	26	34
G001	logging-string-format	15	19	34
PLR1730	if-stmt-min-max	18	15	33
RUF022	unsorted-dunder-all	28	-	28
RET506	superfluous-else-raise	25	-	25
PLR5501	collapsible-else-if	17	6	23
UP015	redundant-open-modes	13	9	22
W605	invalid-escape-sequence	-	21	21
B010	set-attr-with-constant	20	-	20
UP024	os-error-alias	13	7	20
D403	first-word-uncapitalized	15	2	17
D412	blank-lines-between-header-and-content	16	-	16
C420	unnecessary-dict-comprehension-for-iterable	8	8	16
D211	blank-line-before-class	14	-	14
D205	missing-blank-line-after-summary	13	-	13
E701	multiple-statements-on-one-line-colon	13	-	13
E713	not-in-test	5	7	12
PLR1711	useless-return	5	6	11
UP034	extraneous-parentheses	7	4	11
D400	missing-trailing-period	9	-	9
PIE808	unnecessary-range-start	5	4	9
TRY400	error-instead-of-exception	5	4	9
FURB110	if-exp-instead-of-or-operator	2	7	9
D407	missing-dashed-underline-after-section	8	-	8
F541	f-string-missing-placeholders	4	4	8
FURB136	if-expr-min-max	6	2	8
SIM114	if-with-same-arms	2	6	8
E703	useless-semicolon	7	-	7
B009	get-attr-with-constant	6	1	7
D406	missing-new-line-after-section-name	6	-	6
ISC001	single-line-implicit-string-concatenation	6	-	6
RET502	implicit-return-value	5	-	5
PLR0912	too-many-branches	5	-	5
RUF021	parenthesize-chained-operators	4	-	4
UP033	lru-cache-with-maxsize-none	4	-	4
UP018	native-literals	1	3	4
RET507	superfluous-else-continue	2	2	4
UP031	printf-string-formatting	-	4	4
PIE804	unnecessary-dict-kwargs	2	2	4
D301	escape-sequence-in-docstring	-	4	4
W292	missing-newline-at-end-of-file	4	-	4
UP025	unicode-kind-prefix	4	-	4
G003	logging-string-concat	2	2	4
UP039	unnecessary-class-parentheses	3	-	3
UP009	utf8-encoding-declaration	-	3	3
PLW0120	useless-else-on-loop	3	-	3
PLC1802	len-test	-	3	3
SIM910	dict-get-with-none-default	3	-	3
RUF051	if-key-in-dict-del	3	-	3
FURB188	slice-to-remove-prefix-or-suffix	3	-	3
PLR2004	magic-value-comparison	2	1	3
PLR0402	manual-from-import	-	3	3
F841	unused-variable	3	-	3
RSE102	unnecessary-paren-on-raise-exception	-	2	2
F811	redefined-while-unused	-	2	2
RET508	superfluous-else-break	2	-	2
PLR0915	too-many-statements	2	-	2
F632	is-literal	-	2	2
D405	non-capitalized-section-name	2	-	2
FURB105	print-empty-string	-	2	2
SIM108	if-else-block-instead-of-if-exp	1	1	2
TRY004	type-check-without-type-error	2	-	2
RUF100	unused-noqa	2	-	2
ISC002	multi-line-implicit-string-concatenation	2	-	2
RET501	unnecessary-return-none	2	-	2
RET503	implicit-return	1	-	1
SIM117	multiple-with-statements	1	-	1
PLR1733	unnecessary-dict-index-lookup	-	1	1
LOG009	undocumented-warn	1	-	1
G010	logging-warn	1	-	1
B007	unused-loop-control-variable	-	1	1
PGH004	blanket-noqa	1	-	1
D201	blank-line-before-function	1	-	1
Q001	bad-quotes-multiline-string	1	-	1
PLR1736	unnecessary-list-index-lookup	-	1	1
FURB129	readlines-in-for	1	-	1
PLC0207	missing-maxsplit-arg	1	-	1
RUF019	unnecessary-key-check	-	1	1
NPY001	numpy-deprecated-type-alias	1	-	1
Q003	avoidable-escaped-quote	1	-	1
SIM300	yoda-conditions	-	1	1

[spxiwh]

@titodalcanton Here's what was run:

Here are the exact commands, in order:

1. Library files (pycbc/ — commit 4b92dc6)

ruff format pycbc/ bin/
ruff check --fix pycbc/ bin/

Passing bin/ as a directory here only picked up the one .py-suffixed file in it (bin/plotting/pycbc_plot_Nth_loudest_coinc_omicron.py) — ruff's directory traversal filters by extension, so the 221 extensionless scripts weren't touched by this pass at all.

2. bin/ executables (the 221 extensionless scripts — commit cde2d86)

Since those scripts have no .py extension, ruff format bin//ruff check --fix bin/ silently see zero files (same extension-filtering issue). They had to be passed explicitly by path:

# Build the file list: every non-.py file under bin/ with a python shebang,
# excluding the 3 genuine bash scripts (pycbc_copy_output_map,
# pycbc_stageout_failed_workflow, inference/run_pycbc_inference)
find bin -type f ! -name "*.py" ! -name "*.pyc" | while read -r f; do
  case "$(head -1 "$f")" in
    *python*) echo "$f" ;;
  esac
done > bin_python_scripts.txt

ruff format $(cat bin_python_scripts.txt)
ruff check --fix $(cat bin_python_scripts.txt)

Both passes used --fix only (not --unsafe-fixes), so only rules ruff itself marks safe were auto-applied

PLEASE NOTE I then had to do a few small changes:

18460a6
ba2f9ad
5c32566

after this. The first one are because this broke a monkeypatch we had for emcee_pt (so we need to use np.float here). The second one is a circular import, so bad form on our part, and should be fixed properly (we don't want the order of imports in the __init__.py to matter, but moving a certain import to the end avoids an error. The third one was a preexisting typo in documentation which wasn't being correctly picked up as an error before, and was after adding a necessary newline.

[GarethCabournDavies]

Approved, as I have been convinced that this is all safe

[titodalcanton]

@spxiwh should setup.py also be included? Based on your command, I do not think it was.

[titodalcanton]

…and I guess same for /examples/ and /test/

[tdent]

Is there a way to determine which files were affected by PLR2004 magic-value-comparison ? I think this could be nontrivial in terms of understanding the code content - as far as I understand the change, a new CONSTANT variable (named I don't know how) will appear just for the purpose of getting assigned to a numerical value and then comparing with an existing one. This makes the code longer and more complicated and doesn't make it easier to understand because it just shifts the mystery value from one place to another without making a human being explain it properly.