Bulk code cleanliness review — general (non-sector)

[ktehranchi]

Bulk code cleanliness review — general (non-sector)

A full clean-code review of the repository turned up 144 verified findings across 12 domains and 47,895
lines of Python. This issue tracks the 63 of them that are outside sector coupling and carry some
behavioural risk.

The other two buckets are tracked separately:

• Sector-coupling findings → Bulk code cleanliness review — sector coupling #781
• The 57 mechanically-safe cleanups (no behaviour change, mostly deletions) → Mechanically-safe cleanups — 57 items, no behaviour change #782

63 findings — 2 critical, 24 high, 29 medium, 8 low

Why this is worth doing

Raw defect density is almost flat across the codebase — 2.4 to 3.3 findings per thousand lines — so no area is
simply "messier". What the review found is that five habits repeat everywhere, and they have stopped being
stylistic:

1. Returning where it should raise. Functions degrade instead of failing. Because Snakemake caches whatever
   lands on disk, a function that degrades doesn't produce an error — it produces a wrong artifact the DAG then
   treats as a valid, up-to-date input for every downstream rule.
2. The __main__ block is the real program. add_extra_components.py (230 lines), build_demand.py (225),
   build_renewable_profiles.py (350) keep their orchestration inside the guard, where nothing can be imported
   or tested. Functions below reach past their own parameters to module globals.
3. Copy-paste, then one copy drifts. The damage is never the redundancy — it's the divergence.
4. Dead code at scale. ~2,900 lines are provably unreachable, about 6% of the Python in the repo.
5. The safety nets have holes. The mechanisms meant to catch all of the above are themselves compromised.

Critical in this scope

tests/equivalence/report_sections/maps.py:40 — maps.render() is a 567-line function with 12 nested closures and 13 guarded call sites (complexity 54)

This is the reference implementation of the project's own equivalence evidence, and it is the least readable code in the tree. Every helper (three_panel, differing_carriers, mean_pu_by_carrier_bus) is a pure function of pandas/numpy inputs that could be unit-tested in milliseconds, but because they are closures over ctx, nc, na, regs_c, regs_a, labels and norm none of them can be imported or called without a fully built two-sided equivalence run on disk. A change to the diff-panel color scale requires reading 570 lines to find where dlim is set. The _simplified cache at line 84 keys on id(regs), which is only safe because every GeoDataFrame it sees happens to stay alive for the whole call — an invariant nothing states or enforces.

Evidence

line 40:  def render(ctx) -> str:
line 41:      import sys
line 43:      sys.path.insert(0, str(ctx["repo"] / "workflow" / "scripts"))
...
line 606:     return "".join(parts)

Verified: `wc -l` = 606, so render() spans lines 40-606 (567 lines). `awk 'NR>=40 && NR<=606 && /^    def /'` counts 12 nested function definitions (img, plot_regions, three_panel, guarded, per_bus_load, pnom_by_carrier_bus, ext_pnom_max_by_carrier_bus, mean_pu_by_carrier_bus, carrier_vec, carriers_by_total, differing_carriers, plus the section closures _demand/_pnom/_pnom_max/_mean_pu/_profiles/_solved). `grep -c 'guarded('` = 13.

Fix: Lift the pure quantity extractors (per_bus_load, pnom_by_carrier_bus, ext_pnom_max_by_carrier_bus, mean_pu_by_carrier_bus, carrier_vec, carriers_by_total, differing_carriers) to module level taking explicit n / norm arguments, and lift three_panel/plot_regions/img into a small _panels.py taking (plt, np, labels) explicitly. render() then becomes a ~60-line sequence of parts.append(guarded(_section, ...)) calls over module-level section functions _section_demand(ctx, nc, na, regs_c, regs_a) etc. Key _simplified on id(regs) only after documenting the lifetime invariant, or better on a stable attribute the caller supplies. Move the sys.path.insert at line 43 into a module-level import guard so it happens once, not on every render.

_{Risk: low}

workflow/scripts/_helpers.py:363 — is_transport_model returns a ValueError instead of raising it; callers treat it as truthy

A ValueError instance is truthy. Both call sites bind the result and branch on it directly — workflow/scripts/cluster_network.py:937 then if transport_model: at lines 1026, 1114 and if not transport_model: at 1000, 1131; workflow/scripts/prepare_network.py:290 then set_line_s_max_pu(n, transport_model, ...) at 308, whose body is if not transport_model:. So any transmission_network value that is not exactly "reeds" or "tamu" (a typo, a renamed option, a new backend) silently selects the full transport-model branch: line impedances are dropped, s_max_pu is left unset, and clustering takes the reeds path. The run completes, the log is clean, and the numbers are wrong. This is the single highest-cost defect in the domain because it converts a config typo into a silently different physical model.

Evidence

363: def is_transport_model(transmission_network):
364:     match transmission_network:
365:         case "reeds":
366:             return True
367:         case "tamu":
368:             return False
369:         case _:
370:             return ValueError(
371:                 "transmission network not specified correctly. Check config",
372:             )

Fix: Raise instead of return, and make the message name the offending value:

def is_transport_model(transmission_network: str) -> bool:
    match transmission_network:
        case "reeds":
            return True
        case "tamu":
            return False
        case _:
            raise ValueError(
                f"Unknown transmission_network {transmission_network!r}; expected 'reeds' or 'tamu'.",
            )

No call site needs to change — both already assume a bool.

_{Risk: low}

Three further criticals in this scope are one-line, no-behaviour-change fixes and are tracked in
#782: build_renewable_profiles.py:280, workflow/scripts/test/test_land.py:46, and the
repo-root directory literally named " " that holds 100 KB of committed forked code.

Related existing issues

• Accidental addition of workflow/config/config.yaml #771 already covers the accidental workflow/config/config.common.yaml commit. This review adds context:
  because that file sits in the destination init_pypsa_usa.sh refuses to overwrite, init_pypsa_usa.sh is
  permanently broken for every fresh clone — it always prints "Existing config files found… Delete the
  following files and rerun." and seeds nothing.
• Missing top level config values #770 ("Missing top level config values", renewable_land_access) is a symptom of the same problem. The
  two config.common.yaml copies have diverged bidirectionally: the tracked runtime copy has
  renewable_land_access, nrel_caps_reassign and godeeep_wind_height; the repo_data template that
  init_pypsa_usa.sh actually copies from does not. It also pins PUDL v2025.5.0 vs the runtime copy's
  v2025.2.0, and drilling_cost: advanced vs base. Since docs/source/config-configuration.md
literalincludes the template, the published docs describe a configuration the model does not run.

---

Findings

Error handling (22)

• workflow/scripts/_helpers.py:363 — is_transport_model returns a ValueError instead of raising it; callers treat it as truthy
  _{CRITICAL · whole modeling domain · Error handling · risk: low}
• tests/equivalence/report_sections/maps.py:176 — guarded() turns every map failure into HTML prose, so a report "succeeds" with evidence missing
  _{high · whole modeling domain · Error handling · risk: low}
• tests/equivalence/report_sections/stages.py:399 — The report hardcodes benign conclusions about findings it never inspected, in three places
  _{high · whole modeling domain · Error handling · risk: low}
• workflow/scripts/cluster_network.py:1021 — custom_busmap path uses pd.read_csv(squeeze=True), removed in pandas 2.0 — TypeError on pinned 2.2.2
  _{high · power system model · Error handling · risk: low}
• workflow/scripts/opts/policy.py:413 — return instead of continue aborts the whole RPS loop on the first zone with no eligible generators
  _{high · power system model · Error handling · risk: numerics}
• workflow/scripts/plot_validation_production.py:578 — plot_state_emissions_historical_bar raises NameError on historical when the EIA API key is unset
  _{high · power system model · Error handling · risk: low}
• workflow/scripts/retrieve_caiso_data.py:71 — Failed OASIS requests are only printed, yet the missing filename is still queued for reading
  _{high · whole modeling domain · Error handling · risk: low}
• workflow/scripts/retrieve_gridemissions_data.py:32 — Partial download prints and returns; extraction is skipped and the run continues on stale data
  _{high · whole modeling domain · Error handling · risk: low}
• workflow/scripts/solve_network.py:244 — Iterative transmission-expansion branch cannot execute: 1-tuples passed as ints, None return unpacked
  _{high · power system model · Error handling · risk: low}
• workflow/scripts/summary.py:120 — get_energy_timeseries writes into the live links_t frame with a link-name index, raising KeyError
  _{high · power system model · Error handling · risk: low}
• workflow/scripts/zenodo_downloader.py:190 — Downloads stream straight to the final output path and resume on bare exists()
  _{high · whole modeling domain · Error handling · risk: low}
• .github/workflows/main.yml:44 — CI "Setup secrets" step writes an empty ~/.cdsapirc — no secrets are wired into the job
  _{medium · whole modeling domain · Error handling · risk: low}
• workflow/scripts/_helpers.py:699 — CF+ wildcard handling calls packaging.version.parse on a list — always raises TypeError
  _{medium · whole modeling domain · Error handling · risk: low}
• workflow/scripts/add_demand.py:29 — attach_demand checks only row count before overwriting the demand index with network snapshots
  _{medium · power system model · Error handling · risk: numerics}
• workflow/scripts/nrel_exclusion/aggregate_godeeep_weighted.py:104 — Nearest-neighbour bus fill indexes points with positions computed against valid
  _{medium · power system model · Error handling · risk: low}
• workflow/scripts/nrel_exclusion/build_nrel_bus_capacities.py:374 — Output NetCDF attrs record filters as applied even when the tech gate skipped them
  _{medium · power system model · Error handling · risk: low}
• workflow/scripts/opts/policy.py:314 — pd.concat without reset_index makes .loc[row.name] write RPS targets into the wrong rows
  _{medium · power system model · Error handling · risk: numerics}
• workflow/scripts/opts/reserves.py:39 — Three-way except swallow silently restores full ERM capacity credit to fossil in zero-carbon years
  _{medium · power system model · Error handling · risk: numerics}
• workflow/scripts/_helpers.py:548 — mock_snakemake can raise NameError on unbound snakefile and leaves cwd mutated on failure
  _{low · whole modeling domain · Error handling · risk: low}
• workflow/scripts/add_electricity.py:1258 — Dynamic fuel pricing swallows KeyError twice and continues with static costs
  _{low · power system model · Error handling · risk: low}
• workflow/scripts/add_extra_components.py:563 — .mean() or 1 fallback never fires: an empty generator set yields NaN ramp limits, not 1
  _{low · power system model · Error handling · risk: numerics}
• workflow/scripts/opts/bidirectional_link.py:46 — Orphaned fwd/rev links are only reported when no complete pair exists at all
  _{low · power system model · Error handling · risk: low}

Duplication (13)

• workflow/scripts/plot_statistics.py:60 — create_title, save_fig and format_sankey_data are duplicated across modules, largely byte-for-byte
  _{high · power system model · Duplication · risk: low}
• workflow/scripts/test/conftest.py:309 — multi_period_base_network is a ~210-line copy of base_network with three small deltas
  _{high · whole modeling domain · Duplication · risk: numerics}
• tests/integration/conftest.py:46 — _seed_runtime_configs is duplicated in two test roots and both mutate the developer's checkout
  _{medium · whole modeling domain · Duplication · risk: low}
• workflow/rules/postprocess.smk:45 — export_statistics's custom-files branch drops the {simpl} wildcard its sibling rules keep
  _{medium · power system model · Duplication · risk: low}
• workflow/rules/validate.smk:15 — solve_network_validation clones solve_network and declares identical config/log/benchmark paths
  _{medium · power system model · Duplication · risk: low}
• workflow/scripts/_helpers.py:731 — The 15-line carrier-adjustment attr_lookup block is pasted twice, differing in one dict key
  _{medium · whole modeling domain · Duplication · risk: low}
• workflow/scripts/build_fuel_prices.py:155 — generator_name key plus z-score/IQR pipeline is copy-pasted verbatim across two files, comments included
  _{medium · power system model · Duplication · risk: low}
• workflow/scripts/cluster_network.py:673 — The topological_boundaries to bus-region field ladder is written twice inside one function
  _{medium · power system model · Duplication · risk: low}
• workflow/scripts/nrel_exclusion/plot_caps_summary.py:168 — main() inlines a 38-line near-verbatim copy of plot_pnom_map instead of calling it
  _{medium · power system model · Duplication · risk: low}
• workflow/scripts/opts/_helpers.py:58 — filter_components duplicates its four-clause mask and silently returns empty on horizon miss
  _{medium · whole modeling domain · Duplication · risk: low}
• workflow/scripts/opts/reserves.py:45 — Three pypsa constraint builders vendored verbatim with a _RESERVES suffix, dropping upstream terms
  _{medium · power system model · Duplication · risk: numerics}
• workflow/scripts/retrieve_eer_data.py:19 — Two retrieve scripts are the same six statements; one hardcodes its URL, the other takes it from the rule
  _{medium · whole modeling domain · Duplication · risk: low}
• workflow/scripts/scenario_comparison.py:251 — scenario_comparison() re-implements the three modular functions defined above it in 145 lines
  _{medium · whole modeling domain · Duplication · risk: numerics}

Function size (6)

• tests/equivalence/report_sections/maps.py:40 — maps.render() is a 567-line function with 12 nested closures and 13 guarded call sites (complexity 54)
  _{CRITICAL · whole modeling domain · Function size · risk: low}
• workflow/scripts/build_powerplants.py:242 — merge_ads_data: 172-line function that takes a DataFrame but reads its file inputs from the snakemake global
  _{high · power system model · Function size · risk: low}
• workflow/scripts/nrel_exclusion/build_nrel_bus_capacities.py:167 — rollup_supply_curve is a 214-line, complexity-14 function that builds its output Dataset twice
  _{high · power system model · Function size · risk: numerics}
• workflow/scripts/_helpers.py:697 — update_config_from_wildcards is 166 lines, complexity 46, with two unrelated responsibilities
  _{medium · whole modeling domain · Function size · risk: numerics}
• workflow/scripts/add_extra_components.py:958 — add_elec_imports_exports: 179 lines, six nested closures, and a cache whose lookup key never matches
  _{medium · power system model · Function size · risk: low}
• workflow/scripts/cluster_network.py:603 — calibrate_tamu_transmission_capacity is 297 lines mixing file I/O, region resolution and mutation
  _{medium · power system model · Function size · risk: numerics}

Testability (6)

• workflow/scripts/build_renewable_profiles.py:238 — 350 lines of the renewable-profile pipeline live in main with no extractable function
  _{high · power system model · Testability · risk: numerics}
• workflow/scripts/nrel_exclusion/build_nrel_bus_capacities.py:404 — Every nrel_exclusion entrypoint defaults its input paths to one developer's Sherlock home directory
  _{high · power system model · Testability · risk: low}
• workflow/scripts/plot_validation_production.py:259 — Plotting functions read the module-global snakemake inside their bodies, defeating the main() seam
  _{high · power system model · Testability · risk: low}
• workflow/scripts/build_demand.py:2440 — 225-line main is the only wiring layer and none of it can be unit-tested
  _{medium · power system model · Testability · risk: low}
• workflow/scripts/build_demand.py:261 — ReadFERC714 reaches through the class boundary into the global snakemake object
  _{medium · power system model · Testability · risk: low}
• workflow/scripts/cluster_network.py:47 — weighting_for_region mutates n.generators and computes work the population strategy discards
  _{medium · power system model · Testability · risk: low}

Dead code (5)

• workflow/repo_data/config/config.common.yaml:159 — Config keys deleted as dead in PR Change plant/fueltype naming scheme to match pypsa or eia data #10 are back, plus four more no code reads, with false comments
  _{high · whole modeling domain · Dead code · risk: low}
• workflow/scripts/nrel_exclusion/compare_legacy_vs_nrel.py:1 — Three nrel_exclusion modules (573 lines) have zero references anywhere in the repo
  _{high · power system model · Dead code · risk: low}
• workflow/scripts/plot_validation_production.py:282 — Six functions (~290 lines, 28% of the file) are reachable only through commented-out call sites
  _{high · power system model · Dead code · risk: low}
• workflow/rules/retrieve.smk:213 — rule retrieve_ship_raster downloads a file no rule consumes
  _{medium · whole modeling domain · Dead code · risk: low}
• workflow/scripts/_helpers.py:772 — EQ-constraint loop breaks unconditionally, so only opts[0] is ever inspected
  _{medium · whole modeling domain · Dead code · risk: numerics}

Config coupling (5)

• .gitignore:15 — Bare config/ and notebooks/ patterns gitignore the tracked config mirror and all 10 notebooks
  _{high · whole modeling domain · Config coupling · risk: low}
• conftest.py:15 — Root conftest blanket-marks 17 GLPK LP solves as fast, violating the marker's documented contract
  _{high · whole modeling domain · Config coupling · risk: low}
• .pre-commit-config.yaml:1 — Lint config is largely inert: large-file guard excluded where large files live, ruff pins diverge
  _{medium · whole modeling domain · Config coupling · risk: low}
• workflow/Snakefile:115 — POWERPLANTS constant unused; rules hard-code resources/powerplants/ and bypass COSTS
  _{medium · whole modeling domain · Config coupling · risk: low}
• workflow/scripts/build_demand.py:2342 — Profile/disaggregation/scaler are bare strings dispatched in three unrelated chains
  _{low · power system model · Config coupling · risk: low}

Naming (3)

• workflow/scripts/build_demand.py:1731 — _disaggregate_demand_to_buses rebinds load three ways and multiplies by the wrong laf frame
  _{low · power system model · Naming · risk: low}
• workflow/scripts/constants.py:35 — MMBTU_MWHthemal is misspelled, named backwards, and duplicates NG_Dol_MMBTU_2_MWH
  _{low · whole modeling domain · Naming · risk: low}
• workflow/scripts/opts/policy.py:179 — target.name reads the row index, not the CSV name column, so TCT constraints are named by row number
  _{low · power system model · Naming · risk: low}

API surface (2)

• workflow/scripts/add_extra_components.py:1723 — Exports call omits zone_col, silently defaulting to reeds_zone after state/county conversion
  _{high · power system model · API surface · risk: numerics}
• workflow/scripts/_helpers.py:1 — _helpers.py is a junk drawer: 999 lines, nine responsibilities, 45 importers
  _{medium · whole modeling domain · API surface · risk: low}

Magic values (1)

• workflow/scripts/test/fixtures/build_test_network.py:210 — Every wind site's capacity factor is squared — cf_scale is multiplied in twice in p_max_pu
  _{high · whole modeling domain · Magic values · risk: numerics}

---

How this list was produced

A ruff sweep with an expanded rule set (F,C90,ARG,ERA,SIM,RET,PLR,B,PIE,TRY,PTH,DTZ) established an
objective baseline, then twelve reviewers each read one domain of the codebase in full rather than sampling.
Eight of the twelve domains went through an adversarial verification pass instructed to refute: open the file
at the line, confirm the quoted evidence is verbatim, grep the whole repo before accepting any dead-code claim,
and diff both sites before accepting any duplication claim. That pass adjusted 64 findings and confirmed 32.
The remaining four domains self-verified under the same instructions, and the critical findings were re-checked
by hand.

Every item below was confirmed by opening the file at the line. Line numbers are against develop as of
2026-08-24. No code has been changed — this is a survey only.

Each finding carries a risk label:

• safe — provably no behaviour change (mostly deletions)
• low — small blast radius, no numerics
• numerics — touches model results; should not land without a validation run

_{Filed from an automated clean-code review. Items are unchecked survey results, not agreed work — triage
before acting. Ping if you want any line promoted into its own issue.}